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

Mobile UI enhancements #1028

Merged
merged 3 commits into from
Feb 1, 2018
Merged

Mobile UI enhancements #1028

merged 3 commits into from
Feb 1, 2018

Conversation

lpechacek
Copy link
Member

Two practical enhancements recently tested in the field. I'm willing to update manual pages if they get merged.

For now, mapping (fieldwork) zoom level is fixed at 1:2. While this is an established practice for forest maps, it may not make sense for sprint maps.

Copy link
Member

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Thanks for this submission.

Required change:

  • There is a src/gui/map/map_editor.cpp.orig file which needs to be removed from the commits (i.e. please edit the commits).

Extra comments:

  • I have the feeling that the term "GPS" should be avoided in identifiers (and in UI). It is the most popular GNSS but it is not the only one. In Android settings you will hardly find this term.
  • Icon and button position are okay for me at the moment. However in the long term I would prefer to move to a more common "position" icon - the current one looks more like "direction".

bottom_action_bar->addAction(gps_temporary_clear_act, 1, col++);
auto zoom_out_button = bottom_action_bar->getButtonForAction(zoom_out_act);
auto mobile_zoom_out_menu = new QMenu(zoom_out_button);
auto base_scale_action = mobile_zoom_out_menu->addAction(tr("Base scale"));
Copy link
Member

Choose a reason for hiding this comment

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

"Base scale" is a new term in Mapper. How about "1x" (or "100%" if we should switch to percentage)?

connect(base_scale_action, &QAction::triggered, [this]() {
main_view->setZoom(1);
});
auto mapping_scale_action = mobile_zoom_out_menu->addAction(tr("Mapping scale"));
Copy link
Member

Choose a reason for hiding this comment

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

"Mapping scale" is a new term here. How about "2x" (or "200%" if we should switch to percentage)?

I do acknowledge that 200% is the common printing scaling when preparing paper for field work. Does it make sense to introduce this term for devices which are more flexible in scaling than paper?

@lpechacek
Copy link
Member Author

Thanks, Kai, for reviewing the patches and catching the run-away .orig file. I'll push updated patches soon.

Ad "GPS". While searching I could not find a common understandable term for position fix. The most precise would IMO be "satellite navigation position fix" but who would get the meaning. Next one is "(GNSS) position fix" but it sounds much like expert term. Following the Android trace and trying to figure out what terminology Android developers use I pushed the Google button and the first result took me to Android tutorial about Making Your App Location-Aware. Sadly the first sentence refers to "GPS". Next one also talks about "GPS" in the "Device only" mode. IIRC, the same wording is in my Android 7 Location services setting dialog. Therefore I'm for keeping "GPS" on the UI side until we have a commonly understood term.

On the code side, there is already over 330 occurrences of "gps" string in origin/master and 10 files containing with "gps" in its name. While I'm in favor of adopting a more generic name, switching the naming convention only with this commit would be inconsistent. I'm willing to do the renames when there is consensus about the terms to use.

Ad Icon. I like the idea. The current one is a combination of the Pan tool icon (the four blue arrows) and Mapper's position fix symbol (red arrow in the middle). While the current icon is in line with Mapper's outlook, the location graphical idiom might be more understandable for newcomers.

Ad zoom levels. "2x zoom" and "1x zoom" is aligned with the zoom level info toast display. I'd like to keep the word "zoom" in the menu entry to make it larger and therefore easier to hit with stylus. Plain "2x" and "1x" might be too tiny for field work. I'll change the strings.

Regarding 2:1 magnification, it turned out to be useful in my practice as a overmapping prevention. While my device has about 290 PPI and can render 1:15 000 map with amazing details at first glance, 1x is incomparable with printed map and is not legible enough. 2x zoom level turned out to be useful for getting clearer view of the surveyed area. In my current work 2x represents a good compromise between "legible" and "showing the context". I'd be happy to hear other user's voices. If I'm not mistaken @mlerjen had an opinion about fieldwork map scale.

@lpechacek
Copy link
Member Author

related

Implemented as a menu of Zoom Out button.
@dg0yt
Copy link
Member

dg0yt commented Jan 9, 2018

I already used a more common location icon for the "Copy coordinates" pie menu action:

https://github.com/OpenOrienteering/mapper/blob/master/images/copy-coords.png

@ollesmaps
Copy link

Ad "GPS" - just word "Position" might be suitable enough.

@dg0yt
Copy link
Member

dg0yt commented Jan 23, 2018

zoom-action-changes

This proposed change is certainly very useful. But how to deal with regarding v0.8.0 (which still needs updates to Mapper app manual page anyway)? I would like to see/do a few modifications.

Major:

  • The actions "Record temporary GPS trace", "Record temporary GPS position", and "Clear temporary GPS markers" need to remain grouped together.

Minor:

  • Indeed, I prefer the general term "position".
  • I would like to change the position-/track-related icons (standard position marker instead of triangle).
  • The zoom-out popup menu could include the "whole map" action from the top bar. When removing the "whole map" button from the top bar, the grid button could move to the position above the map parts button.

Anyway, position/sensor related actions remain scattered over three corners of the screen (top left, bottom left, bottom right).

@ollesmaps
Copy link

ollesmaps commented Jan 23, 2018

I never use actions "Record temporary GPS trace", "Record temporary GPS position", and "Clear temporary GPS markers". This might be because I use a lot the scribbling into bitmap tool. I wonder if anybody is a heavy user of this?
Standard position marker - you mean the reversed drop? That would be more straightforward, I agree.

@ollesmaps
Copy link

ollesmaps commented Jan 23, 2018

This relates to #952 where "Show whole map - this button shall be rather in the down left corner, where zooming occurs" was also mentioned.

Could you move also the Pan map button a little bit? I would suggest to put it, up and right - to the right side of Zoom in, zoom out buttons. This way one of the button clashes will disappear (close map x pan map, see also #952).

I do not mind having other sensors in upper left corner - these buttons are seldomly used, usually at the start end of work.

@dg0yt
Copy link
Member

dg0yt commented Jan 23, 2018

"Reversed drop" was the word I was looking for :-)

I can't say much about "Record temporary GPS trace", "Record temporary GPS position", and "Clear temporary GPS markers". As I already said at the of my last post, the position features need consolidation, but its out of scope for v0.8.0 / #1028.

Important is to

  • not make the mobile UI even worse.
  • get the UI strings to be translated right, early.

Given that I have this branch checked out the moment I can add a commit with my adjustments for further review.

@dg0yt
Copy link
Member

dg0yt commented Jan 23, 2018

Actually, Android now uses the term "location" in its API. But there is no text anyway in the Android button:

screenshot

@mlerjen
Copy link
Contributor

mlerjen commented Jan 25, 2018

Sorry the question, but where are versions to test?

@dg0yt
Copy link
Member

dg0yt commented Jan 26, 2018

There are no downloads which include this change. I'm waiting for feedback from lpechacek. As soon as I can merge the change, I will update the documentation page and start building the v0.8.0 release (candidate).

@ollesmaps
Copy link

I would argue that making incremantal changes is better than a sudden overhall of UI. Moving buttons within a region is a very small change. It can be tested only in unstable versions. This way we get feedback.

@dg0yt
Copy link
Member

dg0yt commented Jan 26, 2018

If this is a minor change, it should be fine to release it (soon) with the next stable build which is v0.8.0. If it is considered a major change, it won't be in v0.8.0. There is litte reason to provide another preview before v0.8.0.

@dg0yt dg0yt mentioned this pull request Jan 29, 2018
@lpechacek
Copy link
Member Author

Sorry for delayed response - was out sick and kept away from computers in general.

@dg0yt: Good point about icon grouping. I didn't notice it as I made some more changes in my devel branch which shadowed the discrepancy. I also like the term "my location" and the proposal of moving over to "reversed drop" graphical idiom. With that I'm all fine with commit 19454b6.

Regarding the mobile UI as whole, I agree with @ollesmaps that it needs a serious overhaul. While I have made some more changes which I'm happily using in the field, I'm not offering them for merge yet. IMO a big-bang change in direction of mlerjen's proposal in #534 is a better course of action than incremental rework.

@dg0yt
Copy link
Member

dg0yt commented Feb 1, 2018

@lpechacek Glad to hear you are back. I will do the merge and also update the documentation page. (I plan to remove the numbers from the screenshot, now that there are icons.)

It may still take a few more days until I can make a release which includes Android and macOS.

@dg0yt dg0yt merged commit 5c9dbed into OpenOrienteering:master Feb 1, 2018
@dg0yt dg0yt added this to the v0.8.0 milestone Feb 1, 2018
@lpechacek lpechacek deleted the mobile-ui branch February 5, 2018 10:32
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.

4 participants