Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paintroid 608, 609, 610 Created Landing Page with Projects List and Floating Action Buttons #1288

Open
wants to merge 56 commits into
base: develop
Choose a base branch
from

Conversation

Rd4dev
Copy link
Contributor

@Rd4dev Rd4dev commented Jun 10, 2023

PR Description

Jira Tickets

PAINTROID-608: Create Landing Page
PAINTROID-609: Floating action buttons for landing page
PAINTROID-610: Show all projects of the user in the landing page

  • Created a landing page design that closely resembles Catrobat's landing page and Paintroid Flutter's landing page.
  • Added a new image (+) floating action button that opens a new project or a new drawing surface.
  • Added a "Save Project" menu option to the options menu.
  • Clicking on "Save Project" saves the project as a Catrobat-image with the provided filename.
  • When drawing is complete, pressing the back arrow in the navigation or the top toolbar's back arrow overwrites the project to the saved project file.
  • The saved project is stored in the room database and appears in the "My Projects" section of the landing page.
  • Each project is displayed with a thumbnail, filename, and last modified date.
  • Each project offers two options: viewing project details and deleting the project.
  • The thumbnail of the latest project is displayed in the top image preview on the landing page.
  • The "Load Image" (down arrow) floating action button is used to work with existing images.

Your checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

  • Include the name of the Jira ticket in the PR’s title
  • Include a summary of the changes plus the relevant context
  • Choose the proper base branch (develop)
  • Confirm that the changes follow the project’s coding guidelines
  • Verify that the changes generate no compiler or linter warnings
  • Perform a self-review of the changes
  • Verify to commit no other files than the intentionally changed ones
  • Include reasonable and readable tests verifying the added or changed behavior
  • Confirm that new and existing unit tests pass locally
  • Check that the commits’ message style matches the project’s guideline
  • Stick to the project’s gitflow workflow
  • Verify that your changes do not have any conflicts with the base branch
  • After the PR, verify that all CI checks have passed
  • Post a message in the #paintroid Slack channel and ask for a code reviewer

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Jun 13, 2023

A significant number of test cases FAIL due to PerformException when performing click actions. These failures appear to be associated with the presence of enabled device animations and transitions. Running the tests locally with device animations and transitions turned off (as recommended in the Espresso environment setup), the test cases PASS successfully.

Log:

.....
org.catrobat.paintroid.test.espresso.LandingPageActivityIntegrationTest > testImagePreviewAfterProjectInsert[android28(AVD) - 9] FAILED
androidx.test.espresso.PerformException: Error performing 'single click' on view 'Animations or transitions are enabled on the target device.
For more info check: https://developer.android.com/training/testing/espresso/setup#set-up-environment

org.catrobat.paintroid.test.espresso.LayerIntegrationTest > testUndoRedoLayerRotate[android28(AVD) - 9] FAILED
androidx.test.espresso.PerformException: Error performing 'single click' on view 'Animations or transitions are enabled on the target device.
For more info check: https://developer.android.com/training/testing/espresso/setup#set-up-environment
.....

@khalid-nasralla
Copy link
Contributor

khalid-nasralla commented Jun 30, 2023

Hello @Rd4dev ,
imo, the UI is well done but there are still some problems that I encountered while testing your implementation, that need to be checked out again.

I tested the implemented functionalities and saw that sometimes the UI doesnt get updated correctly (e.g. when I delete a project or create one) I think changes to the database arent handled correctly yet in the UI. Also there are still some crashes happening which I encountered, while testing your implementation. (There might be more problems, but these are the ones that I managed to find while testing it)

Some of the things that I noticed (Videos and Pictures):
imageNR gets incremented when no other projects exist
new project name wrong
crash when deleting project
crash when loading non existend file
crash when removing non existend project crashes app sometimes

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Jul 1, 2023

Hi @khalid-nasralla, thanks for the review and inclusion of detailed screenshots and videos, I'll look into the issues and push a fix ASAP!

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Jul 3, 2023

Hi @khalid-nasralla, following is a short description on few fixes I made to the project and would greatly appreciate any feedback.

Here are the details of the fixes:
Issue: 'New project name incorrect' - Notify call called on wrong thread
Cause: The notifyItemInserted call was invoked on the wrong thread.
Solution: Fixed it by calling it in the Main Context.

Issue: 'Crash when deleting project' - Improper exchange of project ID
Cause: There was an improper exchange of project ID.
Solution: Fixed it by retrieving the project directly from the database and adding it to the projectList for UI updation.

Few other fixes were implemented to address various issues:
Issue: Incorrect creation of new files for each overwrite save due to improper project name assignment.
Solution: Reassigned the project name properly to resolve the problem. getImagePreviewUri

Issue: UI not properly updated after updation of project
Solution: Called notifyItemChanged in Main Context along with sorting the list.

Issue: Incomplete deletion of project - Only project from the database deleted
Solution: Implemented two functions, deleteProjectFiles() and deleteProjectImages(), to ensure proper deletion of project files and images.

Fix Requiring Feedback:
Regarding the issue of "imageNR gets incremented when no other projects exist," the project name is set in a same manner to the saveImage implementation. In this implementation, the project name is incremented from the shared preference using the function countUpImageNumber(). However, if a project, let's say image3, is created and then deleted, the next project will still receive the incremented shared preference count, resulting in image4. Changing this behavior to check for project existence and decrementing accordingly (e.g., reverting to image3) may not be appropriate in cases where projects are deleted in between. For instance, if we have image3, image4, image5, image6, image7 and if image4 gets deleted, decrementing to image7 [as shared preference would have been holding 8 and decremented to 7] would lead to the overwrite of files. Or may be the deleted count can be set to the current shared preference image4 for the next file but that would still mess the next coming files as image5 image6 so one would have been already holding projects.
Therefore, I would appreciate your input on this matter before proceeding with any appropriate fixes.

Test Cases Added:
Test case added for getProject Dao

Looking forward to receive any feedback and proceed accordingly

@foenabua
Copy link
Contributor

Hey!
I just tested the current build and I seem to have encountered a bug.

Expected behaviour:
Clicking on "Return to LandingPage" should open "Save or Discard" Dialog.

Actual behaviour:
App crashes

Steps to reproduce:

  1. Create new project and save it
  2. Return to landing page
  3. Create new project and draw something
  4. Return to Landing page:
    This results in:
    grafik

Video:
https://github.com/Catrobat/Paintroid/assets/45263782/67584613-cd1c-47ce-ac9b-39b3e6d5574f

I only tested this on the emulator and I need to mention that I had to update the room version to run on macOS.
grafik

Please check if you can reproduce this.

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Jul 12, 2023

Thanks @foenabua for bringing the bug to my attention. I'll look into it and try to get it fixed as soon as possible!

@foenabua
Copy link
Contributor

@Rd4dev also please have a look at the last two Jenkins builds. There are some test cases that fail. They seem to still work locally but not on Jenkins.

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Jul 13, 2023

Hi @foenabua,

  1. I was able to reproduce the bug, it appears that the new project retained a reference to the old project name, leading to file overwriting. However, resetting it to null each time a new project is created resolved the issue.

  2. Regarding the macOS problem, unfortunately I don't have access to a Mac for testing the incompatibility. However, as advised, I updated the room version to 2.40.alpha03 which works as expected with windows too.

  3. Regarding the Jenkins build, I noticed that the testImagePreviewAfterProjectInsert failed in the last two builds. Despite passing the test locally (as you mentioned), I will investigate the latest build for this commit to identify the failing components and address them promptly.

I would greatly appreciate any feedback or suggestions you may have.
Thank you.

@foenabua
Copy link
Contributor

@Rd4dev sorry for the misunderstanding. I didn't mean that you should change the room version. I just mentioned it in case the bug that I encountered was just because of the version I was using if you couldn't reproduce the bug

@foenabua
Copy link
Contributor

foenabua commented Jul 14, 2023

MoreOptionsTests seem to fail because "Advanced Settings" is not displayed on the screen. You would have to scroll down to click it.
grafik

You could create a new function called swipeUp() in OptionsMenuViewInteractions.

fun swipeUp(): OptionsMenuViewInteraction {
        optionsMenu.perform(ViewActions.swipeUp())
        return this
    }

Then call it before clicking Advanced Settings.
grafik

Of course feel free to find your own solution.

Copy link
Contributor

@foenabua foenabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Job 👍

@khalid-nasralla
Copy link
Contributor

khalid-nasralla commented Sep 2, 2023

Hey @Rd4dev I looked over the code and tested out the implementations and I think it is a huge improvement in terms of functionality since the last time I reviewed it. But there are still some small problems that I noticed which need to be fixed, for example:

Tested on API 31,29,23:

  1. Closing project and trying to open it again too fast by clicking on the project, causes error loading the image and crashes.
    1_video
    1_stacktrace

  2. Closing project and trying to open it again too fast by clicking on the thumbnail, causes error loading the image and crashes.
    2_video
    2_stacktrace

  3. When switching to another project and going back to the project list, the user sometimes can see how the most recent project gets put at the top,
    I think this should happen in the background without the user seeing the order of the project list change. (this might also be the problem why 1) and 2) happens if the user tries to change projects little bit too fast)
    3_video

MainActivity.kt:442 : if it is possible to remove the !!-operator and write it in another way, please do so because we try to avoid the "!!"-operator in production code. Using it in tests is fine though.

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Sep 3, 2023

Hi @khalid-nasralla , thanks for re-reviewing the code and providing all assets. I observed the problem occurring during my testing on API 23 in the latest commit, which led to the introduction of downloadManagerIdRemoved. I will reexamine all the mentioned issues and work on a solution promptly.

@khalid-nasralla khalid-nasralla self-requested a review September 3, 2023 23:24
@Rd4dev
Copy link
Contributor Author

Rd4dev commented Sep 6, 2023

Hi @khalid-nasralla ,

As you mentioned, issue number 3 appeared to be the root cause of problems 1 and 2. When the back/home button is pressed, the activity is terminated, and the project list update occurs in real-time. To address this, I ensured that the activity is only finished after the project is saved by utilizing the existing Save Image callback method. This adjustment allows the update and project save processes to occur in the background. I hope this resolves the crashing issues. I'd appreciate any feedback and look forward to working on additional fixes if necessary.

@juliajulie95
Copy link
Contributor

I found another bug, please take a look.
It works when I type in a new name, but if I use the suggestion it somehow messes up the other file.
https://drive.google.com/file/d/1oUQnyUzLA7jzjX6Ir9e8pX8q_07lhqcX/view?usp=sharing

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Sep 21, 2023

Thanks for the review @juliajulie95! I will look into the cause and provide a fix ASAP!!

@Rd4dev
Copy link
Contributor Author

Rd4dev commented Oct 3, 2023

Hello @juliajulie95, it appears that the problem stems from the storeImagePreviewUri variable retaining previous values.
This issue has been addressed in the most recent commit, and I believe this will resolve the matter.

@aman021202
Copy link

Kindly assign me this issue.

@Rd4dev Rd4dev requested a review from foenabua January 31, 2024 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants