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

Fix zoom ratio calculation #364

Merged
merged 11 commits into from
Jun 27, 2018
Merged

Fix zoom ratio calculation #364

merged 11 commits into from
Jun 27, 2018

Conversation

ulteq
Copy link
Contributor

@ulteq ulteq commented Dec 29, 2017

This simplifies the logic behind the automatic zoom ratio calculation, which is used by both --auto-zoom and --scale-down.

When merged this will:

  • Improve the performance (especially in fullscreen mode)
  • Fix the w command when both --scale-down and --keep-zoom-vp are enabled
  • Fix --auto-zoom not being triggered on window resize events when --scale-down is enabled
  • Fix --auto-zoom not being applied to the first image
  • Fix --auto-zoom conflicting with manual zoom
  • Fix --zoom <percent> --auto-zoom logic according to the man feh description
  • Fix feh_draw_checks not taking the zoom level into account properly
  • Fix feh_draw_checks being called before the zoom level is adjusted
  • Prevent --zoom <percent> from blocking --scale-down in fullscreen / fixed geometry mode
  • Prevent --keep-zoom-vp from blocking the dynamic window resizing mechanism
  • Prevent automatic recalculation of the zoom ratio when --keep_zoom_vp is enabled
  • Prevent winwid-> im_x and winwid-> im_y from being set to invalid values
    (when switching between images of different sizes)
  • Simplify the way --keep-zoom-vp is handled internally

Fixes: #229
Fixes: #244
Fixes: #307

Closes: #358
Closes: #361
Closes: #362

@derf
Copy link
Owner

derf commented Jan 9, 2018

Hmm. I'm happy that someone is attempting a cleanup of this mess, though this still needs some testing (and, possibly, polishing).

Right now the first slide of a feh --fullscreen slideshow is not centered properly. I'll let you know if I find anything else (or come up with a patch for it, in which case I'll happily merge the request unless I find other issues).

Expect delays though, I don't have much time at the moment.

@ulteq
Copy link
Contributor Author

ulteq commented Jan 10, 2018

though this still needs some testing

I absolutely agree.

Expect delays though

No problem. We don't have to rush this.

@ulteq
Copy link
Contributor Author

ulteq commented Jan 10, 2018

Right now the first slide of a feh --fullscreen slideshow is not centered properly.

int had_resize = winwid->had_resize || resize; solved it.

@ulteq ulteq force-pushed the simplify-zoom branch 4 times, most recently from a78debb to 1171a50 Compare January 17, 2018 08:49
@ulteq
Copy link
Contributor Author

ulteq commented Jan 28, 2018

@derf This is ready for a second review.

@derf
Copy link
Owner

derf commented Mar 8, 2018

hmm, this still causes some behaviour changes in feh. fullscreen mode no longer zooms out images which are too large, and due to the re-use of the geometry flags settings leaving fullscreen mode will cause non-tiling feh windows to be as big as the screen (instead of fitted to the size of the image, which is usually smaller). Also, in non-tiling mode, feh will no longer resize its window when changing images.

I've yet to decide which of these I consider to be an issue and which I don't (and do more testing), so this is just the current status to make sure i don't forget it.

@ulteq ulteq force-pushed the simplify-zoom branch 3 times, most recently from 4b4c501 to 7245722 Compare March 9, 2018 15:22
@ulteq
Copy link
Contributor Author

ulteq commented Mar 9, 2018

fullscreen mode no longer zooms out images which are too large

I modified this condition in order to replicate the old behavior: 9d959f0#diff-24777376c7590882b07457017fd284ceR459

Also, in non-tiling mode, feh will no longer resize its window when changing images.
leaving fullscreen mode will cause non-tiling feh windows to be as big as the screen

Both of this happened because I enabled fixed geometry mode by default in order to avoid the flickering in tiling windows: 203e8ff#diff-24777376c7590882b07457017fd284ceL326

I have now replaced this code change with a slightly modified version of the original code: 66a1eb8

Flickering in tiling window mode can now be circumvented by passing an empty string to the --geometry option. This makes feh launch with fixed geometry mode enabled. We could also add a new --fixed-geometry option instead.

ulteq added 10 commits March 10, 2018 21:30
This simplifies the logic behind the automatic zoom ratio calculation, which is used by both --auto-zoom and --scale-down.
Passing an empty string to the --geometry option will enable fixed geometry mode without having to specify anything else
--keep-zoom-vp will no longer block the dynamic window resizing mechanism.
@ulteq ulteq force-pushed the simplify-zoom branch 2 times, most recently from e7050e0 to f7363e3 Compare March 10, 2018 21:20
@ulteq
Copy link
Contributor Author

ulteq commented Jun 27, 2018

Please let me know if there is anything I can do to help you make this pull request ready for merge.

@derf
Copy link
Owner

derf commented Jun 27, 2018

Actually, just poking me again was sufficient. I couldn't find any issues with this version :)

Thanks a lot for the patches and sorry for the long wait!

@derf derf merged commit 701cf13 into derf:master Jun 27, 2018
@ulteq ulteq deleted the simplify-zoom branch July 15, 2018 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants