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

Minor fixes for the map window #198

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Conversation

lukehorvat
Copy link
Contributor

Hi all. My first patch for the EL client. (Thanks Ben for helping me with building the client on my Mac)

This PR proposes fixes for 3 small bugs in the map window. The fixes are a bit opinionated, so please let me know if you think they should be solved in different ways.

Bug 1

If you start adding a mark and then move to a different map, the temporary (yellow) mark will still show on the new map:

map.change.before.mp4

This feels a bit buggy to me, because the mark (coords + label) was only relevant to the previous map. It doesn't make much sense for the temporary mark to persist over to the next map imo.

This PR fixes it by clearing the temporary mark on map change:

map.change.after.mp4

Bug 2

If you start adding a mark and then select a different map via the map window, the temporary mark will still show on the new map. But if you then press Enter to save the mark, it will inexplicably disappear. You have to go back to the map where you originally started adding the mark to see that it was actually saved there:

map.marker.save.before.mp4

Again, kinda buggy and not intuitive. Considering you can't even right-click on maps your character is not currently on, you should not even be able to see the temporary mark on the new map, let alone save it.

This PR fixes it by clearing the temporary mark when clicking the small map in the map window:

map.marker.save.after.mp4

Bug 3

If you select a different map via the map window, and then have your character actually move to that same map, the map window does not respond to interaction like you'd expect it to:

map.preselection.before.mp4

This is because the map window goes into a non-interactive state the moment you select a different map, even if your character later moves to that map. You'd have to clear the map selection first by double-clicking the small map to bring back interactivity. Annoying.

This PR fixes it by clearing the map window's currently-selected map on map change:

map.preselection.after.mp4

This fix feels like a bit of a nuclear option, however I think/hope most players will intuitively understand that moving to a different map "resets" the map window...

@pjbroad
Copy link
Collaborator

pjbroad commented Jan 23, 2024

@lukehorvat its likely far too late to talk about this patch, sorry about that, but I've just taken another look. When I looked at this way back when you submitted it, I was not sure that I agreed with all of it. Then I stopped working on the client for a while and forgot about it. Second time round, its seams like a good compromise. The only thing I'd consider adding is to clear and current input text, not just hide the yellow cross; but I'm not sure. Anyhow, I'm merging the PR now so thanks for providing it and sorry it was left unanswered.

@pjbroad pjbroad merged commit 8f89a53 into raduprv:master Jan 23, 2024
@lukehorvat
Copy link
Contributor Author

Thanks @pjbroad. Just happy to see it land :)

@lukehorvat lukehorvat deleted the fix-map-markers branch February 2, 2024 23:23
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