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

Improvements for view positioning when scrolling #791

Merged
merged 8 commits into from
Mar 18, 2024
Merged

Conversation

karlch
Copy link
Owner

@karlch karlch commented Mar 17, 2024

Adds a new setting scroll_to_center that controls whether the view is always centred when scrolling along with the :move-view command that can position the view at top / center / bottom with the zt / zz / zb keybindings.

karlch added 6 commits March 17, 2024 10:43
When True, always keep the cursor centered while scrolling as is the
current behaviour. Otherwise, cursor ends up at top / bottom when
scrolling up / down.
The command moves the view, keeping the cursor on the same position, in
library and thumbnail mode.
@jcjgraf
Copy link
Contributor

jcjgraf commented Mar 17, 2024

zt, zz and tb work as expected.

The scroll_to_center option does not seem to have an impact for me (I tried to set it dynamically using :set scroll_to_center XXX). No matter that I set the option to, the view is never centered.

This is probably, because in widget::scrollTo, hint is not as expected None, but ScrollHint.EnsureVisible.

On top of that, I was able to observe two crashes.
I changed the scroll_to_cente option dynamically, and afterwards I scrolled in the thumbnail view:

[10:58:31] ERROR    <vimiv>              Uncaught exception! Exiting gracefully and printing stack...
Traceback (most recent call last):
  File "/home/jeanclaude/Documents/Programming/vimiv-qt/vimiv/gui/thumbnail.py", line 458, in resizeEvent
    self.scrollTo(self.currentIndex())
  File "/home/jeanclaude/Documents/Programming/vimiv-qt/vimiv/widgets.py", line 35, in scrollTo
[10:59:56] ERROR    <vimiv>              Uncaught exception! Exiting gracefully and printing stack...
  Traceback (most recent call last):
  File "/home/jeanclaude/Documents/Programming/vimiv-qt/vimiv/gui/thumbnail.py", line 458, in resizeEvent
    self.scrollTo(self.currentIndex())
  File "/home/jeanclaude/Documents/Programming/vimiv-qt/vimiv/widgets.py", line 35, in scrollTo
    super().scrollTo(index, hint)
TypeError: scrollTo(self, index: QModelIndex, hint: QAbstractItemView.ScrollHint = QAbstractItemView.EnsureVisible): argument 2 has unexpected type 'NoneType'

I hope this helps. I do not have time to look into the code atm. But let me know if you need anything else.

Btw, I like the option of having a not-centered view a lot (thanks for the idea @Markuzcha and the implementation @karlch) 😊

@karlch
Copy link
Owner Author

karlch commented Mar 17, 2024

Thanks @jcjgraf for testing this so quickly! I can reproduce both, hopefully I find the time this afternoon for a detailed investigation 😊

EDIT: Quick fix seems to fix the setting, but I have weird issues with zz / zt / zb now, needs more investigation later.

karlch added 2 commits March 17, 2024 11:51
None is not a valid scroll hint and the default is EnsureVisible in Qt.
Thus, the previous behaviour:
1) Never enforces the scroll_to_center setting, as the hint was never
   None.
2) Crashed, when no scroll hint was passed (as in thumbnail when called
   directly with vimiv).
@karlch
Copy link
Owner Author

karlch commented Mar 17, 2024

@jcjgraf mind giving this another shot? I am no longer able to reproduce any issues with zt / zz / zb I had, unsure if there were unstaged changes or I messed up somehow 😕

@Markuzcha
Copy link

I just gave it a shot @karlch - It works like a charm !
Bot zz / zt / zb and the scroll_to_center setting, without crashes or error messages in the command line whatsoever

Thank you very much, it's even more pleasure to use it now.

(PD.: Would be great to have it updated in the AUR as well.)

@jcjgraf
Copy link
Contributor

jcjgraf commented Mar 18, 2024

@karlch Everything works as expected 🎉

@Markuzcha It is in the AUR git package as soon as this is merged, and in the general AUR package, once a new version of vimiv gets released.

@karlch karlch merged commit 9edf362 into master Mar 18, 2024
28 checks passed
@karlch karlch deleted the scroll-hints branch March 18, 2024 16:22
@karlch
Copy link
Owner Author

karlch commented Mar 18, 2024

Awesome, thanks to both of you for testing! 😊

As @jcjgraf mentioned, the changes are now available via the AUR git package.

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.

3 participants