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

added --begin-top option to force view point at the top of the image #278

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

0xmatthias
Copy link

Feh automatically shows the centre of the image after zooming into the image. This option forces feh to set the view point after zooming at the top.I also comment out one if-else case that causes extreme fast switching between the same image (first big then small, if auto-zoom is on in sideshow and the user skips between images).

@ulteq
Copy link
Contributor

ulteq commented Dec 30, 2017

I also comment out one if-else case

This will break automatic window resizing for any non tiling window manager.

@0xmatthias
Copy link
Author

Thanks for clarifying it, I revert the if-else case change, so everything should work as usually if non tiling window managers are used.

@0xmatthias
Copy link
Author

Added a second argument --static-window to disable automatically window resizing if a new image is displayed, cause otherwise the image will be displayed sometimes twice if a tiling window manager is used. First big for a few milliseconds, then small (because the wm resizes the window).

@ulteq
Copy link
Contributor

ulteq commented Mar 4, 2018

I get to see the flickering you describe only when I manually disable the "fixed geometry mode" (by pressing the toggle_fixed_geometry key 'g' once) while the window dimensions are force overriding by a tiling window manager like i3.

@0xmatthias
Copy link
Author

I am using a tiling window manager and I have the exact opposite effect. If I run feh with a bunch of images e.g.
$ feh *.jgp
I have flickering, if I switch between them. When I press 'g' once then the flickering is completely gone, but the image size is scaled down to fit into the window size.
I don't want to scale down the image size so that it fits into the windows width and height (like using --scale-down).
Pressing 'g' once and start feh with --scale-down has the same effect.

btw I think the description for -g is not correct. From the manpage -g, --geometry [width x height] [+ x + y] the parameter width x height and +x +y are complete optional, that means -g without anything should be totally ok. But when I run
$ feh -g ~/pictures/image.jpg
feh tries to read all images in the actual directory instead of showing ~/pictures/image.jpg. My best guess is that feh interprets '~/pictures/image.jpg' as an argument for -g.
If I run -g with another option, then the next option is completely ignored, e.g.
$ feh -g -d ~/pictures/image.jpg
Shows image.jpg but doesn't print the filename, but
$ feh -g -d -d ~/pictures/image.jpg
shows image.jpg and print the filename.

@ulteq
Copy link
Contributor

ulteq commented Mar 5, 2018

You're right! My bad. I was testing while my default theme was active. I can now reproduce the issue.

I think this pull request is going to fix your issue: #364

Could you give it a try?

btw I think the description for -g is not correct.

We should create a new ticket for that issue: https://github.com/derf/feh/issues

@0xmatthias 0xmatthias force-pushed the show_top_with_zoom branch from a6a0f37 to c097db9 Compare March 5, 2018 19:58
@0xmatthias 0xmatthias force-pushed the show_top_with_zoom branch from c097db9 to 4ec93c2 Compare March 5, 2018 20:02
@0xmatthias
Copy link
Author

0xmatthias commented Mar 5, 2018

#364 indeed fixes the flickering issue, well done!
I completely removed the static-window option.
Therefore my pull request only contains the new option to force the view point at the top of the image.
I also opened a new issue #386 to address the wrong description for -g

@ulteq
Copy link
Contributor

ulteq commented Mar 6, 2018

What do you think about giving the user even more options to specify the alignment of the image?

Like top_left, top, top_right, ..., center. Or maybe --align [top, bottom] x [left, right]

@derf
Copy link
Owner

derf commented Mar 6, 2018

Something like --inner-geometry with X11 geometry strings and a special center argument might be useful. The present --keep-zoom-vp code should be able to handle most of that already.

@ulteq
Copy link
Contributor

ulteq commented Mar 6, 2018

I think the image alignment should happen here: https://github.com/ulteq/feh/blob/simplify-zoom/src/winwidget.c#L452-L453

Something like --inner-geometry with X11 geometry strings

Sounds great.

@0xmatthias
Copy link
Author

Sounds great.
I don't have much time at the moment. Will try to implement it at the weekend.

@derf
Copy link
Owner

derf commented Mar 8, 2018

I had a convenient combination of time and motivation today and implemented it in master, see the commit referenced above. It adds --inner-geometry +X+Y with +0+0 == top left, +0-0 == bottom left, -0+0 == top right and -0-0 == bottom right. Non-zero arguments are also possible, e.g. +0+100 to skip the first 100 pixels of each image (but only if it's larger). It's not perfect, but should be an acceptable solution until the inevitable refactoring of the whole zoom / rendering code. Please tell me what you think.

This will probably make merging #364 slightly more work for me and/or @ulteq (sorry about that), but that's still better than piling up even more open pull requests I guess :)

Do any of you see a case where the width_x_height part of the geometry string would be useful? I don't, and if you don't see one either I'll rename the option to --offset or similar and discard width and height. That would probaly make it more intuitive, too.

@ulteq
Copy link
Contributor

ulteq commented Mar 9, 2018

Do any of you see a case where the width_x_height part of the geometry string would be useful?

No.

I'll rename the option to --offset or similar

Would have been my suggestion as well.

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