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

Re-add quick navigation with comma and period keys #3799

Merged
merged 6 commits into from Mar 29, 2020

Conversation

@Noordfrees
Copy link
Member

Noordfrees commented Mar 24, 2020

I don't remember the details how these used to work in b19, so I can't guarantee that it works exactly the same. Should be roughly similar though and feels intuitive at least to me.

Fixes #3229

@Noordfrees Noordfrees added this to the build21-rc1 milestone Mar 24, 2020
@Noordfrees Noordfrees self-assigned this Mar 24, 2020
@gunchleoc

This comment has been minimized.

Copy link
Contributor

gunchleoc commented Mar 25, 2020

Code LGTM, not tested.

…yboard-quicknavigation
@stonerl

This comment has been minimized.

Copy link
Member

stonerl commented Mar 25, 2020

The old behavior to keep the last 10(?) in a stack and you could jump between them.

@simply-peachy

This comment has been minimized.

Copy link

simply-peachy commented Mar 25, 2020

This seems to work consistently when strictly jumping to Inbox events and the minimap. However, the following behaviour currently acts in a way that seems weird:

  1. Start at HQ
  2. Jump to an Inbox event, or click minimap
  3. Scroll view with the mouse
  4. Use comma key to replay previous locations
    Instead of jumping to 2 then 1 it seems to jump to some other places I've recently scrolled to with the mouse.

Also the previous behaviour would "record" a location when one manually scrolls to a location (after a scrolling certain distance). That way, manually moving around to several places, far away, can be replayed easily. Thanks for your efforts so far :-)

@Noordfrees

This comment has been minimized.

Copy link
Member Author

Noordfrees commented Mar 26, 2020

Not sure if I understand correctly. So I start at A, jump via the minimap to B, scroll with the mouse to C, jump with the minimap to D. Now the desired comma behaviour would be:

  • If B and C are far apart: D→C→B→A
  • If B and C are close by: D→C→A (or D→B→A?)

What distance would be a reasonable threshold? (note: for distance calculation at this level we only have pixel distance and zoom level available, no coords)

@Noordfrees

This comment has been minimized.

Copy link
Member Author

Noordfrees commented Mar 26, 2020

Pushed a fix now that the start and end point of map dragging will both be considered if the cumulative drag distance is greater than a quarter screen rect. Is this better now?

@simply-peachy

This comment has been minimized.

Copy link

simply-peachy commented Mar 26, 2020

Not sure if I understand correctly. So I start at A, jump via the minimap to B, scroll with the mouse to C, jump with the minimap to D. Now the desired comma behaviour would be:
* If B and C are far apart: D→C→B→A
* If B and C are close by: D→C→A (or D→B→A?)

That's how I recall the previous behaviour. I would assume the logic behind it would be that a small scroll is easy to return to manually, but if you've scrolled a long way you might not remember precisely where you were, and it would take a lot of scrolling to get back there.

What distance would be a reasonable threshold? (note: for distance calculation at this level we only have pixel distance and zoom level available, no coords)

A quarter of the screen sounds sensible.

Copy link

simply-peachy left a comment

Tested with this commit, seems to be working great now! \o/

@gunchleoc gunchleoc merged commit 5338497 into widelands:master Mar 29, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Noordfrees Noordfrees deleted the Noordfrees:readd-keyboard-quicknavigation branch Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.