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

Remember tile_size/zoom factor between play sessions #4423

Merged
merged 9 commits into from Oct 16, 2019

Conversation

@Wedge009
Copy link
Member

commented Oct 2, 2019

Resolves #1518

I don't know if this is the 'right' way to do things - there are a lot of places where preferences are set. There is a display preferences class and a display class, but also most things seem to be recorded in general preferences class. tile_size is referred to in game_config.cpp and the actual zooming is done inside the display class, using game_config::tile_size as the DefaultZoom. The default tile_size seems to be hard-coded to 72 in a few places.

Regardless, this seems to work for me.

@Wedge009 Wedge009 added the UI label Oct 2, 2019
@Wedge009 Wedge009 requested a review from jostephd Oct 2, 2019
src/display.cpp Outdated Show resolved Hide resolved
@@ -233,6 +233,8 @@ display::display(const display_context * dc, std::weak_ptr<wb::manager> wb, repo

set_idle_anim_rate(preferences::idle_anim_rate());

if(preferences::tile_size() > 0)
zoom_ = preferences::tile_size();
zoom_index_ = std::distance(zoom_levels.begin(), std::find(zoom_levels.begin(), zoom_levels.end(), zoom_));

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 2, 2019

Member

Here, zoom_ is an unsigned int and elements of zoom_levels are doubles:

wesnoth/src/display.hpp

Lines 251 to 255 in b593e82

/**
* Function which returns the size of a hex in pixels
* (from top tip to bottom tip or left edge to right edge).
*/
static int hex_size(){ return zoom_; }

zoom_levels = 0.25, 0.33333333333333333, 0.5, 0.75, 1.0, 1.25, 1.5, 2.0, 3.0, 4.0

Which of these would be more suitable to save in the preferences file?

This comment has been minimized.

Copy link
@Wedge009

Wedge009 Oct 3, 2019

Author Member

I stuck with zoom_/tile_size since that seems to be the figure that's actually used in the scaling calculations. Also storing integers as text seems to be cleaner than floating-point values (especially considering that one-third value). I didn't really consider zoom_levels when looking at how to implement this (again, not really sure what the 'right' way is since the relevant pieces of code seems to be scattered across several files).

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 3, 2019

Member

Agree about doubles. I just wonder if saving the hex_size by itself is sufficient, or whether we should save both hex_size and the reference value (72 pixels). After all, #1322 implies that it's supposed to be possible to change the game_config::tile_size.

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

The default tile_size seems to be hard-coded to 72 in a few places.

It's ostensibly supposed to come from game_config (data/game_config.cfg and the corresponding game_config class), but yeah.

Question. What if the #1213 (comment) scenario happens after we merge this? That is, suppose that somebody zooms in by accident (cat walked on their keyboard or something) and can't figure out how to zoom back out. Today, they can restart wesnoth and it'll be fine. With this PR, that will no longer work. Is this a problem?

If it is, maybe we should fix not only #1213 but also #1518, for example, by also adding a slider to the preferences dialog as both of those issues suggested, or by adding menu items such as "Zoom in" "Zoom out" "Reset to default zoom" as #1518 suggested. In either case, that would be in addition to what you've already implemented (adding preferences::tile_size and preferences::set_tile_size and calling them when the zoom is changed), not instead of it. What do you think?

@Wedge009

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

There is always the option to reset the zoom to 100% (via default hot-key 0) but in the cat-on-keyboard scenario suggests that the user is likely unaware of these options to begin with. Maybe just because I do know about the zoom feature I'd be in favour of saving vs not saving the zoom level but that's not a hard opinion.

Regarding the related UI enhancements you mentioned, they would certainly be 'nice to have'. I don't really know about UI stuff, though. The menu, I could imagine being the right-click context menu or the top-bar menu in-game.

I suppose this could be postponed until those other UI enhancements come into effect, I don't really know either way. Depends on how much value people would get from this - the requester for #1518 (neverenough) seemed to want this to avoid having to redo the zoom with every play session so presumably it's worth something.

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Regarding the related UI enhancements you mentioned, they would certainly be 'nice to have'. I don't really know about UI stuff, though. The menu, I could imagine being the right-click context menu or the top-bar menu in-game.

Adding stuff to the menus is pretty easy. First, add it one of the [menu] tags in the theme:

items=objectives,statistics,unitlist,statustable,save,savereplay,savemap,load,preferences,chatlog,menu-autosaves,help,stopnetwork,startnetwork,surrender,quit,quit-to-desktop

Then, map the string to a HOTKEY_* constant:

{ HOTKEY_CREATE_UNIT, "createunit", N_("Create Unit (Debug!)"), false, scope_game, HKCAT_DEBUG, "" },

And that's it. There's already a hotkey 0 for resetting the zoom, so the HOTKEY_* constant for it is already implemented; it just needs to be added to the Menu menu, or the context menu.

I think making "Reset zoom" easier to find is all that's needed for #1518. Making the "zoom in" and "zoom out" options easier to find, as #1213 requests, can be done separately. How does that sound?

@Wedge009

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

I refactored the zoom validation and applied it to initialisation as well as setting. I checked that it works, including putting an 'invalid' tile_size in the preferences: it rounds to the nearest 'valid' zoom setting.

I also applied the zoom options to the context menu.

Have I missed anything?

@Wedge009

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

Noting that #1636 is probably a better way of resolving #1213 than this but is beyond what I'm trying to do here.

src/display.cpp Outdated Show resolved Hide resolved
src/display.cpp Outdated Show resolved Hide resolved
@Wedge009 Wedge009 force-pushed the Wedge009:record_zoom branch from 6c06b81 to 7cb8a29 Oct 8, 2019
@jostephd jostephd self-requested a review Oct 8, 2019
Copy link
Member

left a comment

Thanks for your patience as this sat in my todo list.

src/display.cpp Outdated Show resolved Hide resolved
@Wedge009 Wedge009 force-pushed the Wedge009:record_zoom branch from cf296fb to 6560fdb Oct 14, 2019
@Wedge009

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2019

No worries, I'm in no hurry.

Why does it keep complaining about conflicts in the change log when I've re-based it? :S

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

Why does it keep complaining about conflicts in the change log when I've re-based it? :S

There were several changes to changelog after the commit you had rebased onto:

$ git log --oneline --reverse 6560fdbd74cb37738ef40ec3166672b593887bf4..origin/master changelog.md
e495d795b59 Add :terrain to changelog
980fad6bf8f Add #4410 to changelog
2c2d1f8b2fa Replays: Show visible units in "Damage versus" sidebar tooltip
4c646d51d99 changelog: #4410 has been backported
bc40366d78c Add #4407 to changelog
854e2cd2f0d Add UtBS update to changelog
0d9730d14af Add #4396 to changelog.

I'm not sure what happened here, but I guess that you rebased onto master but hadn't brought master up to date with origin/master first. I usually do git rebase origin/master instead.

@Wedge009 Wedge009 force-pushed the Wedge009:record_zoom branch from 6560fdb to 0f52ebd Oct 14, 2019
@Wedge009

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2019

I just realised I was fetching from Wedge009/master instead of origin/master. Whoops. Should be okay now.

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

Yeah, GitHub's happy now. #4423 (comment) still stands, though.

@Wedge009

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2019

Yes, I haven't got around to it yet.

@Wedge009 Wedge009 force-pushed the Wedge009:record_zoom branch from 0f52ebd to a88fae4 Oct 15, 2019
@Wedge009

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2019

Okay, I think I've finally got something that will satisfy both of us by making zoom_index_ be what governs the zoom operations here instead of trying to work off zoom_. I've also done a lot more thorough testing:

  • No value in preferences
  • Empty value in preferences
  • Non-numeric value in preferences
  • Non-integral number in preferences
  • Integer smaller than MinZoom in preferences (including negative values)
  • Integer in between zoom_levels values in preferences
  • Integer above MaxZoom in preferences

All work as intended. And I think the code reads a lot better now (but I may be biased).

[ci skip]
@jostephd jostephd self-requested a review Oct 16, 2019
src/display.cpp Outdated Show resolved Hide resolved
src/display.cpp Show resolved Hide resolved
[ci skip]
src/display.cpp Show resolved Hide resolved
* It's not a set function in the normal sense so the old name was confusing.
@Wedge009 Wedge009 merged commit fdf1785 into wesnoth:master Oct 16, 2019
0 of 3 checks passed
0 of 3 checks passed
label label
Details
Travis CI - Pull Request Build Errored
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@Wedge009 Wedge009 deleted the Wedge009:record_zoom branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.