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 scroll to selection & Hide iconview when changing window width #482

Merged
merged 3 commits into from May 14, 2021

Conversation

kbengs
Copy link
Member

@kbengs kbengs commented May 5, 2021

No description provided.

@jeromerobert
Copy link
Member

Hiding the iconview make resizing smoother but for some reason, the iconview takes some time to be back. Sometime the iconview remains grey until I move the mouse on the window.

@kbengs
Copy link
Member Author

kbengs commented May 8, 2021

Sometime the iconview remains grey until I move the mouse on the window.

I haven't been able to reproduce that. But I have done some small changes that should make iconview visible a bit faster. The mouse button release event and enter notify event are not emitted always, but for theese cases the timeout should make iconview visible again. @jeromerobert can you still see the problem?

It does take some time for iconview to get visible again after changing window width, but at least for me iconview is ready for interaction faster with this commit than before. For example on a 3670 page pdf, when resizing by double-clicking on headerbar iconview is ready for interaction in about 4 seconds, where it before took about 8 seconds to get ready. And resizing window by dragging window edge is almost impossible in main for such large pdf.

@jeromerobert
Copy link
Member

jeromerobert commented May 8, 2021

It does take some time for iconview to get visible again after changing window width

As far as I can see, it's not "changing", but only increasing. Reducing the width is always fast. When I increase the width I often (i.e. not always) have a 4s delay before the iconview gets back. It does not depends on the size of the PDF file as I can reproduce it with a 10 pages PDF files.

I will take a closer look (i.e. add some trace) to understand why this is happening.

@jeromerobert
Copy link
Member

enter_notify_event is always triggered when decreasing width. enter_notify_event is sometime, but not always, triggered while increasing width.

@jeromerobert
Copy link
Member

I don't know if that can help but, as far as I can see, the only event which is always called with resizing end is GDK_FOCUS_CHANGE (i.e. window_focus_in_out_event).

@jeromerobert
Copy link
Member

Adding self.set_iconview_visible(timeout=False) to window_focus_in_out_event fix the issue. I wonder if we need to keep set_iconview_visible in window_enter_notify_event and window_button_release_event.

@kbengs kbengs force-pushed the fix-scroll_to_selection branch 2 times, most recently from 7c7eee0 to 3f01e2b Compare May 9, 2021 06:33
@kbengs
Copy link
Member Author

kbengs commented May 9, 2021

Okay thanks. On my machine window_focus_in_out_event is not triggered when changing width so it seems we need to have all three events.

@jeromerobert
Copy link
Member

When starting with the window already maximized the iconview remains grey. self.iconview.get_columns() is then -1 so it leads to an infinit loop in get_visible_range2(). render() should exit if self.iconview.get_columns()<=0.

When switching from maximize to unmaximize the iconview remains grey for a while. I'm trying to find the right additional events where to call set_iconview_visible.

@kbengs
Copy link
Member Author

kbengs commented May 9, 2021

When starting with the window already maximized the iconview remains grey

I think a len(self.model) > 0 addition will fix this case:

def window_configure_event(self, _window, event):
    """Handle window size and position changes."""
    if self.window_width_old not in [0, event.width] and len(self.model) > 0:

@jeromerobert
Copy link
Member

jeromerobert commented May 9, 2021

I think a len(self.model) > 0 addition will fix this case:

It does !

Calling set_iconview_visibleon window_state_event fix the grey iconview effect when unmaximizing the window. I cannot find how to fix it when maximizing.

Scroll to selection can sometimes fail because vertical scollbar is
not yet ready for the new value. With this approach the action will
be repeated until it is successfull.
This will make it possible to do smooth width changes even
if there are a large number of pages in iconview.
This is if self.iconview.get_columns() return -1
@codecov-commenter
Copy link

Codecov Report

Merging #482 (2b3eba5) into main (abb3f02) will decrease coverage by 0.07%.
The diff coverage is 72.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
- Coverage   73.72%   73.64%   -0.08%     
==========================================
  Files          10       10              
  Lines        2793     2827      +34     
==========================================
+ Hits         2059     2082      +23     
- Misses        734      745      +11     
Impacted Files Coverage Δ
pdfarranger/pdfarranger.py 67.39% <72.09%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abb3f02...2b3eba5. Read the comment docs.

@kbengs
Copy link
Member Author

kbengs commented May 13, 2021

I can see that it sometimes takes 1,5s longer before iconview gets visible again (when timeout make it visible). This is for example when pressing maximizing and unmaximizing buttons on headerbar. I can't find a good way to fix it. What desktop environment do you see the longer delays on @jeromerobert ? Do you think this delay is acceptable?

@jeromerobert
Copy link
Member

It's with XFWM4/XFCE4. Well yes, it's acceptable like this.

@jeromerobert jeromerobert merged commit 234d5a9 into pdfarranger:main May 14, 2021
@kbengs kbengs deleted the fix-scroll_to_selection branch September 12, 2021 14:00
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.

None yet

3 participants