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-419 "Vanishing" UI controls #1180

Merged
merged 3 commits into from
Mar 31, 2023

Conversation

Lenkomotive
Copy link
Contributor

@Lenkomotive Lenkomotive commented Jan 3, 2023

PAINTROID-419

  • 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

Last Comments made in #1106 where i picked up from:

  1. Zooming in and out seems to not work 100% correct, as after each zoom, the first click the user makes seems to be ignored and doesn't change anything on the canvas, the second click then shows the bars, which were vanished, again.

  2. When using the line tool and drawing a line, then clicking on plus to extend the line, the top bar vanishes when clicking on the plus button, which should not happen, as this is not happening, when user clicks undo, redo and checkmark, which are all also in the top bar but when clicked, the bar does not shortly vanish.

  3. Overall single clicking with any tool does not seem to work as i.e. with the brush tool no dot is drawn and when single clicking and keeping the finger on the screen for a short amount of time, something is drawn but not a single dot but rather a small short line. This most likely happens because of vanishing the bars, which also slightly moves the canvas and therefore a small line rather than a dot is recognized. This is actually not really acceptable as users are not able to make small changes with this bug.

  4. Smudge tool seems to be not really usable with this feature, at least when I'm trying to smudge a small portion of the display just for a bit, the handling of the clicks are to vague to make proper changes. One option would be to disable this feature for some tools, where the constant switch between vanishing and displaying is rather disturbing than helpful, like in this tool. But this should also be discussed in further detail.

  5. Also if I understood Wolfgang correctly, the canvas shall not be maximized when clicking "hide buttons" in the settings menu. This is now the case, as well as when exiting the "hide buttons" mode.

  6. It seems like the test: testTransformToolSetCenterFarCenter() fails since this PR, so please try to fix this before I can approve changes.

New in this PR:

1.+ 3. Every touch event caused the statusbar to hide. Respectivly the drawingSurface changed its position. Now the app hides the statusbar by default.
positive: bigger drawing surface , all errors from 1 and 3 are solved, no unnecessary overhead for calculating the correct recentering of the drawing surface.
negative: user can see status bar only when swiping down

2.Wrapped the line-handling logic in a new method, so no animations are triggered when clicking the button

4.deactivated animations in smudge tool
-> redesign of smudge tool menu needed because it uses too much space

5.removed resetScaleAndTranslation() call from hideButtons calls

6.This tests fails locally also in delvelop branch.

7.The animations were not working correctly in landscape mode, this was also fixed.

  • adjusted testcases (no autoscrolltask anymore)

@Lenkomotive Lenkomotive closed this Jan 3, 2023
@Lenkomotive Lenkomotive reopened this Jan 3, 2023
@Lenkomotive Lenkomotive force-pushed the PAINTROID-419 branch 6 times, most recently from 83585e5 to c21f4c3 Compare January 3, 2023 11:39
@Lenkomotive Lenkomotive force-pushed the PAINTROID-419 branch 2 times, most recently from d737222 to d985a8d Compare February 7, 2023 16:03
@Lenkomotive Lenkomotive changed the title Paintroid 419 PAINTROID-419 "Vanishing" UI controls Feb 7, 2023
@SebastianGrief
Copy link
Contributor

Some things I noticed:

  • The tool menu doesn't disappear on draw, which is the same behaviour as before, but kind of wierd now with the bars disappearing. Also not the best behaviour in the first place imho.
    Reproduce: Pick a tool -> reopen the tools menu -> draw
    Maybe @juliajulie95 can comment on whether we want to open a new ticket for that.

  • The reappearing isn't working as intended (I think?). For me they do not slide in but pop up and slide to their end position.
    So the animation (slideUp, slideDown) should depend on whether the topBar and bottomNavigation are appearing or disappearing.

Copy link
Contributor

@SebastianGrief SebastianGrief left a comment

Choose a reason for hiding this comment

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

See comment

@juliajulie95
Copy link
Contributor

Please fix the tool window not disappearing (@SebastianGrief mentioned it above)

@Lenkomotive Lenkomotive force-pushed the PAINTROID-419 branch 5 times, most recently from ea0d508 to a21e077 Compare February 26, 2023 13:49
@Lenkomotive
Copy link
Contributor Author

Both findings are fixed now. @SebastianGrief @juliajulie95

Copy link
Contributor

@SebastianGrief SebastianGrief left a comment

Choose a reason for hiding this comment

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

👍

@ThorstenBandel
Copy link
Contributor

There are more tests than normally failing. The following tests do not fail on develop. Could you take a look at those?

  • testTransformToolSetCenterFarCenter
  • testShapeToolBoxGetsPlacedCorrectWhenZoomedIn
  • testTextToolBoxIsPlacedCorrectlyWhenZoomedIn
  • testTextToolDoesNotResetPerspectiveScale

@Lenkomotive Lenkomotive force-pushed the PAINTROID-419 branch 6 times, most recently from 887f824 to eef8272 Compare March 26, 2023 20:01
FromG and others added 2 commits March 27, 2023 16:39
 hide unhide tool options with animation.

 if the tool has top and bottom specific layouts. they will slide up and down.
 tools with only bottom layouts slide down.
 -added methods in every tool effected by the animation
 -also added methods in the default option views.

 -changed Drawinsurface that the drawing surface will begin at the top of the screen instead from under the topnavigationLayout and will end at the bottom of the screen instead of the navigationbottomlayout.

-changed headers of worked on files.

-removes scrollingviewintegrationtests

-removes the auto scrolling feature. when you draw the drawingsurface will not scroll with the current direction the user draws.

-also hides the android status and navigation bar.

-remove autoscrolltasktests

- removed non full screen mode. the user will now always be in the fullscreen perspective.

- does not reset scale when you scale out and then draw on the zoomed out image.

- only resets scale when you switch modes.
@Lenkomotive Lenkomotive force-pushed the PAINTROID-419 branch 3 times, most recently from 0e5af13 to 91940da Compare March 27, 2023 18:52
@Lenkomotive
Copy link
Contributor Author

Lenkomotive commented Mar 28, 2023

testTransformToolSetCenterFarCenter is fixed now and should not fail anymore. The other three are also failing on develop sometimes, looks like there might be timing issues sometimes.

Copy link
Contributor

@ThorstenBandel ThorstenBandel left a comment

Choose a reason for hiding this comment

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

Thanks!

@ThorstenBandel ThorstenBandel merged commit 63e6899 into Catrobat:develop Mar 31, 2023
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