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

Add filmstrip option in thumbnail mode #336

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from
Draft

Conversation

karlch
Copy link
Owner

@karlch karlch commented Feb 6, 2021

As code speaks louder than works, here is a draft PR for what I meant with using thumbnail mode for the described listview in #299.

Thumbnail mode can now also be shown at the right of the current image as a simple list similar to the filmstrip used by other programs. In addition, the display of thumbnail mode can now be any combination of icon and text, with the default being icon-only in icon view, and icon + text in list view. The view mode is switched by the thumbnail.listview boolean setting. Toggling is currently bound to the <tab> key. Showing the icon is changed using the thumbnail.display_pixmap setting (should probably be renamed to display_icon). Showing the text is changed using the thumbnail.display_text setting. Each of these two setting can have one of the following options:

  • never: never show the related element
  • always: always show the related element (default for the icon)
  • listview: only show in listview (default for text)
  • iconview: only show in iconview

What do you think of the general concept @jcjgraf ?

In case we go for this, I see some more todos:

  • Simplify switching between options showing text / icon, possibly using keybindings or completely refactoring the concept
  • Clean up code addressing anything marked TODO and more
  • Add tests
  • Update documentation, probably with a screenshot for the new list view

@karlch karlch marked this pull request as draft February 6, 2021 17:15
@jcjgraf
Copy link
Contributor

jcjgraf commented Feb 7, 2021

This is golden! It adds exactly what I was missing from vimiv; a synchronized list, displaying all images opened from multiple directories, in thumbnail view next to image view.

Thank you very much for this work!

You are right, code speaks louder than words. In #299 I though I meant to integrate this view directly into the library, which would have not solved the initial problem of #299. This implementation is somewhat similarly to what I meant. But I did not expect it to be that "easily" integrable.

I have only three things minor things to add:

  • Toggling this module is bit cumbersome. While in image mode it required gt, TAB, gi to open it, and the same in reverse to close it again. I think a command or binding would be appropriate for that. Especially, since I am not sure if it is possible to add a custom binding for the toggling because of the required TAB.
  • When the thumbnail listing is open and the thumbnails are rather large, it can happen that the listing overlay the image even though if the image was shifted to the left, both would fit next to each other without interference. Since this may not easily fixable, we can also leave it as it is.
  • Lastly, except for the largest thumbnail size, image names are often cut of. Maybe we could split the name in two lines or something like that. Though, I don not really mind leaving it as it is as I do rather not display the filename anyways 🙃

@jcjgraf
Copy link
Contributor

jcjgraf commented Feb 11, 2021

I have noted a inconsistency in the synchronisation between thumbnail and image mode. I.e. in image mode we see image A while thumbnail listing mode shows image B as active.

Reproduce

  • Open some image in image mode (call it image A)
  • Switch to thumbnail mode (normal mode, not the new list)
  • Select another image (call it image B)
  • Press tab to open the thumbnail listing

Expected Behaviour

  • Image mode and thumbnail listing mode both show image B

Actual Behaviour

  • Image mode shows image A
  • Thumbnail listing mode shows image B

Note

Synchronisation is respored after switching to the next image

@karlch
Copy link
Owner Author

karlch commented Feb 14, 2021

Thanks a lot for your detailed comments! Glad you like the general idea.

  • I agree toggling is currently cumbersome. In principle one could add a custom keybinding like enter thumbnail && set thumbnail.listview true && enter image. Do you think it would make sense to have a default binding for this? If so, any preferences? I was thinking of gv as gl is already taken. And do you think it should always enter image mode, or should it stay in the thumbnail view?
  • Understandable, but this sounds like it may be more effort than gain. If it is easy to fix, I will add it.
  • Ah, yes, the issue makes perfect sense, although I propose another solution. There is the thumbnail.font style option and I think a much smaller default than the overall font makes sense here. We could also allow the text to be wider than the actual thumbnail. What do you think?
  • The synchronization issue makes perfect sense, image mode never follows the other modes as loading images is expensive. I would not change this behavior. Instead I think it makes sense to add a flag to open-selected which decides whether or not thumbnail mode should be closed, similar to the library. Like this one could restore synchronization by pressing a key without leaving thumbnail mode. But I don't really have a key in mind, is there anything intuitive you can think of? <space> is one that came to my mind, but that is a bit of a stretch.

EDIT: I think I misunderstood, the expected synchronization would be to select the correct image in the listview, not in image mode, right?

@jcjgraf
Copy link
Contributor

jcjgraf commented Mar 1, 2021

I am very sorry for the long delay with answering and that I have not worked on #325.

  • Um, that must have been a late-evening though of myself. Obviously, one can easily create such a binding since tab itself is a binding 😆. I would propose gf for a binding since many other application call such a view "filmstrip". Personally, I thing it would be useful to end off in image mode after using this binding. Though, I can also understand when people prefer staying in thumbnail mode. Maybe it is a bit overkill, but why not adding a boolean setting which determines if the binding is enter thumbnail && set thumbnail.listview true or enter thumbnail && set thumbnail.listview true && enter image respectively?
  • No worries, I am fine with how it is...
  • That is a possible solution too, though I am not sure if it is sufficient to decrease thumbnail.font size to make filenames of "average length" fit. Allowing the text to overflow the thumbnail box may work in filmstrip view, but when the image names are displayed in the thumbnail view, image names may easily overlap. We could certainly allow the text to be wider than the thumbnail in filmstrip view only, though I am not sure it this would look neat anyways. I am fine with slightly decreasing the thumbnail.font size and else, leave as it is. We can still solve this somehow, sometimes in the future if it turns out to be an issue in the long run.
  • My expected synchronisation was that when I select an image in thumbnail mode and then open the thumbnail listing (press TAB), that the image displayed in image mode is the one I selected in thubmanil mode. I have attached a GIF which hopefully helps to show you my point.

gif

@karlch
Copy link
Owner Author

karlch commented Mar 7, 2021

Absolutely no worries, I am also currently rather busy and will probably not find the time to work on this in the next few weeks unfortunately.

  • gf sounds good to me, but I am not a fan of having a setting for the entering or not. How about having two bindings, e.g. gf and gF where gf would just enter the filmstrip mode (which is more consistent with the other gx bindings) and gF would open it and then enter image mode. Maybe there is also a better binding combination than this.
  • Concerning the fontsize, let's see how this goes, your concerns are all valid and this will require some playing around with the various use-cases.
  • I am not quite sure how to best synchronize here. I guess loading the currently selected image when entering "filmstrip" mode could be a valid option. But the image would certainly not "follow" any scrolling in filmstrip mode by default, much like it doesn't follow the library selection unless explicitly requested. I guess we should add n and p here as well as a binding to open the selected image without leaving filmstrip mode.

@jcjgraf
Copy link
Contributor

jcjgraf commented Mar 12, 2021

  • I like gf and gF. I have also thought that we could create just one binding and depending if the current mode is thumbnail or image, the binding would end in thumbnail or image mode respectively. Though, separate bindings may be more powerful and less confusing.
  • This makes all sense. I think loading the currently selecting image when opening the film strip would mostly solve the issue I guess. I am not sure what you mean with adding the bindings n and p.

karlch added 20 commits June 3, 2021 15:12
When in icon mode, the regular thumbnail view is shown, i.e. it is shown
instead of the image with a grid of thumbnails.

When in list mode, a single column of thumbnails is displayed at the
right of the current image. The background is rendered semi-transparent.
It is now possible to also show the basename of the thumbnail element in
thumbnail mode as well as hiding the pixmap. For each of the two we have
four options:
- always show (default for pixmap)
- always hide
- show only in icon mode
- show only in list mode (default for text)
Instead of using an alpha factor we reduce lightness and saturation.
This keeps the effect of unfocusing seperate from alpha which is
required for the listview.
The width is now the maximum of the icon size or the current text width.
The height is now the sum of the visible content, i.e. icon and text.

In addition, the icon is no longer perfectly centered in case text is
visible in the bottom, precisely to account for the text.
The new bindings `gf` and `gF` enter filmstrip mode (and open the
current image in image mode). In addition the `gt` binding now forces
entering thumbnail mode in "icon" mode.
Equivalent to the library, they scroll in addition to opening the
current thumbnail in image mode. This is especially useful in
"filmstrip" mode.
The constructor for a thumbnail item slightly changed.
This essentially reverts
cd2f570
but we keep a new style option `thumbnail.font` for the thumnbail font
to allow customizing the size.
`zi`/`zI` show/hide the icon.
`zn`/`zN` show/hide the name.
This better reflects the gf related bindings and keeps the naming
consistent.
This allows running chains which depend on a new mode, like the new `gF`
binding.
Instead of setting "when the elements are visible" we now set "what is
visible in each mode". This now leaves us with the two settings
`thumbnail.display_icon` and `thumbnail.display_filmstrip`. Each of them
has the valid options `icon`, `name` and `both`.

This has a few advantages compared to the previous implementation:
- It is not possible to show nothing.
- It is easy to toggle through the valid options.

In addition, the logic checking if a specific element is visible in the
current mode has been put into properties to simplify this in each part
of the code.
When running
:set thumbnail.filmstrip!
or
:set thumbnail.filmstrip false
it seems logical to hide the filmstrip when not in thumbnail mode.
Entering thumbnail mode seems a bit of a stretch as we never auto-enter
modes.
@karlch karlch force-pushed the thumbnail-listview branch from 7961f62 to 52a8095 Compare June 3, 2021 22:07
@karlch karlch changed the title Add listview option in thumbnail mode Add filmstrip option in thumbnail mode Jun 3, 2021
@karlch
Copy link
Owner Author

karlch commented Jun 3, 2021

All right, this took a while but finally there is some more progress 😊

  • The two bindings have been added, although gF ends with open-selected instead of enter image to partially address the synchronization issue.
  • There was quite some progress on the rendering, mainly the height and width of thumbnail elements now adapt to the visible content. Let me know if you encounter anything weird, this was quite some havoc 😆
  • I have added n and p in thumbnail mode, you can best test them when in filmstrip view (and in thumbnail mode). I am still not convinced we should display the corresponding image immediately, but may just go for it, although not tonight 😄

In addition to all this, the settings have changed in the behaviour, i.e. we now set "what is visible in mode x" and not "when is x visible". This has a few advantages as described in 86219bd. Mainly, it is now possible to jump through the different display options. This is currently bound to w without any particular reason except for it being easy to type and close to <tab>. Let me know if you can think of a meaningful binding 😊 I also adopted the "filmstrip" naming you recommended already for the gf binding.

With all the changes, I hope I didn't break anything. Any additional feedback is more than welcome as always 😊

Copy link
Contributor

@jcjgraf jcjgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you late-evening shift, I like a lot what you have done 👍

I have gone through you changed. While I did not understand every single line (esp. QT related things), I have still added a few comments. Two of them indicate bugs which happen only when you use rather long file names. I hope I have described everything well enough, I you do not understand something, let me know.

Additionally, I think we have a little mess with the naming of all these modes/view. I.e. we have thumbnail mode, icon mode, list mode, filmstrip view, (grid mode) as well as combinations with other modes (image mode + filmstrip view).
I presume that this can be rather confusing for users (at least it is for me). While from the code base, icon mode and filmstrip mode are similar (both depend on the thumbnail mode) they are in practice different things. I do rather see the filmstrip mode as an additional view of image mode instead of a view of thumbnail mode.

I think a layer of abstraction would here be appropriate; Image mode has like three views:

  • Normal (like image mode before this PR)
  • Filmstrip open (filmstrip open but image mode activated)
  • Filmstrip active (filmstrip open and active)

Image mode is entered using gi. From there, one can toggle between Normal and Filmstrip open view using tab. From any mode (/view) Filmstrip active can be accessed using gf. When using gi from any mode, we get to Image Normal or Image Filmstrip open depending on the toggle of filmstrip (i.e. depending on thumbnail.filmstrip).

Thumbnail mode would be equivalent to how it was before this PR (with the exception of the option to display the image name).

As said, while this change may not make sense from the POV of the code base, I belief it improves the user experience. Let me know what you think.

ViewOption,
"thumbnail.display_filmstrip",
ViewOption.Both,
desc=f"Which element(s) to display in icon view, one of {_view_options}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Which element(s) to display in filmstrip view


_view_options = quotedjoin(option.value for option in ViewOption)

display_icon = EnumSetting(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setting display_icon is a bit ambiguously named, how about display_grid?

@@ -56,12 +56,10 @@ def __init__(self, *colors: str, font: str = DEFAULT_FONT):
self.check_valid_color(color)
self[f"base{i:02x}"] = color
# Fill in all default values
self["font"] = font
self["font"] = self["thumbnail.font"] = font
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing thumbnail.font via styles works, but why do you set self["thumbnail.font"] = font but why do you not do it for e.g. statusbar.font or library.font?

@@ -39,7 +39,6 @@
"statusbar.message_border": "2px solid",
"statusbar.padding": "4",
# Thumbnail
"thumbnail.font": "{font}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand the full styles stack, but I have the feeling that this line should not be removed 🤔

if api.modes.current() != api.modes.THUMBNAIL:
self.hide()
self._update_geometry()
self.rescale_items()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to call self._update_geometry() after calling self.rescale_items(). Else the width of the cell gets not updated adequately.

This happens e.g. when in icon view you display the icon + name and the name is much wider than the image and in filmstrip mode you only display the icon.

width = self.iconSize().width()
if self.name_visible():
height += self.text_size.height()
width = max(width, self.text_size.width())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefers if in filmstrip view the width is bounded to the width of the image and not the max of image with and text width.
This has the advantage zooming in/out has direct effect on the width of the filmstrip. Also, we do not get any unnecessary overlay of the filmstrip over the image view when there are only some images with long names.

Obviously, this causes the name to get clipped, which is not so nice. But I would still prefer to have it this way.

I.e. something like

if self.name_visible():
    height += self.text_size.height()
    width = max(width, self.text_size.width()) if (self.viewMode() == self.IconMode or width == 0) else width

api.signals.all_images_cleared.connect(self.clear)
api.signals.new_image_opened.connect(self._select_path)
api.signals.new_images_opened.connect(self._on_new_images_opened)
api.settings.thumbnail.size.changed.connect(self._on_size_changed)
api.settings.thumbnail.filmstrip.changed.connect(self._on_view_changed)
api.settings.thumbnail.display_icon.changed.connect(self.rescale_items)
api.settings.thumbnail.display_filmstrip.changed.connect(self.rescale_items)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only calling self.rescale_items() upon change of thumbnail.display_icon and thumbnail.display_filmstrip respectively, is not enough. Calling self._update_geometry() is also required. Else, the width of the cells are not updated correctly. This happens e.g. when in icon view you display the icon + name and the name is much wider than the image and in filmstrip mode you only display the icon.

@karlch karlch added this to the v1.0.0 milestone Sep 4, 2021
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