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

View does not move upon consecutive slide deletion #501

Closed
Scafir opened this issue Jun 30, 2021 · 10 comments
Closed

View does not move upon consecutive slide deletion #501

Scafir opened this issue Jun 30, 2021 · 10 comments
Assignees
Labels
UX UI/UX (User Interface / User Experience)
Milestone

Comments

@Scafir
Copy link
Contributor

Scafir commented Jun 30, 2021

Desciption

The view does not move upon consecutive slide deletion. This is a problem when deleting many slides, because we may end up way further than the last deleted slide.
What happens is that when we delete slide number N to slide number N+K, the view stays at slide number N+K, which is now an unseen slide.

To Reproduce

Steps to reproduce the behavior:

  1. Open any long document.
  2. Select many consecutive slides and delete them.
  3. We end up ahead in the document.

Expected behavior

I would expect that when we delete slide number N to slide number N+K, the view moves at slide number N, so that we can see the splice.

Input files

N/A

Screenshots

N/A

System and Versions

  • PDF Arranger version 1.7.1, flatpack
  • Pikedpdf version: Not installed. Included in the flatpack?
  • Fedora 34
@dreua

This comment has been minimized.

@angsch
Copy link
Member

angsch commented Jun 30, 2021

I am not sure if there is something intricate with different pages sizes, but the page deletion does not have a call to scroll_to_selection.

    def on_action_delete(self, _action, _parameter, _unknown):
        """Removes the selected elements in the IconView"""

        self.clear_selected()
+       self.scroll_to_selection()

@Scafir
Copy link
Contributor Author

Scafir commented Jun 30, 2021

Do all your pages have the same dimensions? To me it looks more like the scroll-area is too long if the pages have different dimensions than an error when deleting pages. Actually, the scroll area doesn't need to be longer than necessary in order to make the scroll-slider easier to use. If that is fixed, the scroll-back is fixed as well.

All pages dimensions are the same. I am not sure I correctly the second part of your comment though...
Another thing I should have mentioned is that my view is zoomed a lot, so all slides are in a single column and I can only see two or three at a time (so I can take a good look at them).
Here is the document, if it happens to be useful.

@dreua
Copy link
Member

dreua commented Jul 2, 2021

Sorry for the confusion, it seems like I found another issue while trying to reproduce yours and didn't take enough care reading your report. I'll hide my earlier comment and create another issue for what I found.

I'll also take @angsch 's suggestion as a PR, which works great by the way, and commit it straight to main. Thanks to both of you 👍

@dreua dreua added the UX UI/UX (User Interface / User Experience) label Jul 2, 2021
@dreua dreua added this to the 1.8.0 milestone Jul 2, 2021
@dreua dreua self-assigned this Jul 2, 2021
@dreua
Copy link
Member

dreua commented Jul 2, 2021

Looks like I fell straight into a rabbit-hole here: The self.scroll_to_selection() is nice for big one column views, but I don't like it for cases with low zoom level (big grid of small thumbnails) where the destination is already in view and the scrolling is unnecessary, unexpected and distracting. Unfortunately the methods I tried to avoid that don't work. A proper fix is beyond my available time right now, any help is welcome.

@kbengs
Copy link
Member

kbengs commented Jul 2, 2021

One possible way to do it could be to read if the selected page is in the visible range get_visible_range2() and only scroll_to_selection() if it is not in visible range. But a more proper fix would maybe be to fix 506 first.

@dreua dreua removed their assignment Jan 9, 2022
@dreua
Copy link
Member

dreua commented Jan 9, 2022

I'll need to revisit this, #609 should have made this much easier, thanks @kbengs !

@dreua dreua self-assigned this Jan 9, 2022
@kbengs
Copy link
Member

kbengs commented Jan 22, 2023

@dreua are you working on this or do you mind if I try to fix it?

@dreua
Copy link
Member

dreua commented Jan 22, 2023

I intended to but apart from the first findings here didnt, sorry. Im happy if you take it :)

kbengs added a commit to kbengs/pdfarranger that referenced this issue Jan 23, 2023
@dreua
Copy link
Member

dreua commented Feb 1, 2023

Thank you!

kbengs added a commit to kbengs/pdfarranger that referenced this issue Feb 14, 2023
@jeromerobert jeromerobert added this to the 1.10 milestone Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UX UI/UX (User Interface / User Experience)
Projects
None yet
Development

No branches or pull requests

5 participants