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

PaintOnTemplateFeature: Use menu instead of dialog #1710

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

dg0yt
Copy link
Member

@dg0yt dg0yt commented Aug 28, 2020

... for selecting and templates.

(Split from GH-1706. Needs style udpdates for acceptable usability on Android.)

scribbling-3
Template selection moved to scribble mode button (press-and-hold). Only templates in the current viewport, relative position indicated as icon.

@dg0yt dg0yt mentioned this pull request Aug 28, 2020
Let the list order represent the layering again.
This reverts commit 0e207a7
which was never part of a release.
@dg0yt dg0yt force-pushed the scribble-template-list branch from 6249e93 to c8c78cf Compare August 29, 2020 19:21
@dg0yt
Copy link
Member Author

dg0yt commented Aug 30, 2020

@lpechacek wrote in #1706 (comment):

  • I failed to familiarize myself with the new draft selection dialog. When I long-click the scribble tool button with stylus, the menu appears right below the stylus. As I cannot read it, I instinctively lift the stylus from the display which, however, immediately selects a template.

  • I don't understand the icons showing overlap between the draft and viewport. Looks like the more white there is, the more overlap. I'd expect the opposite. The more colored icon, the more overlap.

@dg0yt dg0yt force-pushed the scribble-template-list branch from c8c78cf to afa42e0 Compare August 30, 2020 09:02
@dg0yt
Copy link
Member Author

dg0yt commented Aug 30, 2020

LGTM now:

Unbenannt

@dg0yt dg0yt requested a review from lpechacek August 31, 2020 06:51
Copy link
Member

@lpechacek lpechacek left a comment

Choose a reason for hiding this comment

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

Code-wise the change looks good to me. I'll test the new menu ASAP and provide feedback.

*
* The functor captures the provided viewport, and creates icons which
* represent the template area as white rectangle relative to a gray
* rectangle representing the viewport.
Copy link
Member

Choose a reason for hiding this comment

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

The comments needs updating or rewording. The template area is now grey on the icon surface if I read the code correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -227,20 +276,26 @@ void PaintOnTemplateFeature::initTemplateListWidget(QListWidget& list_widget) co

auto& map = *controller.getMap();
auto const viewed_rect = viewedRect();
auto const icon_width = list_widget.style()->pixelMetric(QStyle::PM_SmallIconSize);
auto const make_icon = makeIconBuilder(icon_width, viewed_rect);
Copy link
Member

Choose a reason for hiding this comment

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

Suggest a different variable name. icon_builder = makeIconBuilder(... would read smoother.

Copy link
Member Author

@dg0yt dg0yt Sep 1, 2020

Choose a reason for hiding this comment

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

Done

FTR, the name make_icon was chosen with regard to the spot where it is used. The other function should have been called makeMakeIcon :-)

Visualize relative template position.
... for selecting and adding templates.
@dg0yt dg0yt force-pushed the scribble-template-list branch from afa42e0 to a1349b3 Compare September 1, 2020 04:28
Copy link
Member

@lpechacek lpechacek left a comment

Choose a reason for hiding this comment

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

Just tested the change on my tablet. Looks pretty good to me and I appreciate the enhancement. Thanks!

@dg0yt dg0yt merged commit fed35cd into master Sep 2, 2020
@dg0yt dg0yt deleted the scribble-template-list branch September 2, 2020 04:01
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.

2 participants