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 support for moving items up/down 10 rows and to top or bottom of … #1

Closed
wants to merge 1 commit into from

Conversation

@snarkophilus
Copy link
Contributor

@snarkophilus snarkophilus commented Oct 16, 2020

…a reading list.

@JimmXinu
Copy link
Owner

@JimmXinu JimmXinu commented Oct 16, 2020

Found a few bugs.

If I select several books and click up/down 10 or beginning/end, I end up with blank rows appearing in the list. Here's a simple way to see it (Win10, Calibre 64 bit):

  1. Add four items to an empty list
  2. Edit List
  3. Select top 3 items
  4. Click 'Move to bottom'
  5. Watch empty rows appear
  6. Click 'Move to bottom' or 'Move to top' more times to see more weirdness.

Looping on move_rows_up/down() is inelegant and inefficient for large lists, especially with large selections. I had it hang for several seconds with less than a 1000 entry list. Many calibre users have libraries of 10s of thousands.

There's a comment in move_rows_up(self) that says:

        # Workaround for strange selection bug in Qt which "alters" the selection
        # in certain circumstances which meant move down only worked properly "once"

I wonder if that's part of it?

@snarkophilus
Copy link
Contributor Author

@snarkophilus snarkophilus commented Oct 23, 2020

For the first bug, blank rows appearing, I can reproduce that - probably just need to add a bounds check? This was obviously a case I didn't check. Ta, will look into this.

For the second bug/comment, yes, looping is inefficient. I see a small delay with my current 300ish list. This was the least invasive way to implement this. I'd also wager than people with a 1000 entry list wouldn't move items or selections of items through lists, at least not without breaking their mouse button :).

Last - I think I should have created a branch then applied the patch in my local git repo? Still new git.

@snarkophilus
Copy link
Contributor Author

@snarkophilus snarkophilus commented Oct 23, 2020

Jim- for the first bug, I can reproduce the problem with the existing (unmodified) plugin with the same steps, except clicking on the down arrow button repeatedly. With four books in a list , the first click moves the top three items down to the bottom, the following clicks then insert a blank row per click.

@JimmXinu
Copy link
Owner

@JimmXinu JimmXinu commented Oct 23, 2020

So it does. Joy.

I think I figured it out and pushed a fix: f9ab494

You want to try that with your code?

@snarkophilus
Copy link
Contributor Author

@snarkophilus snarkophilus commented Oct 24, 2020

So it does. Joy.

I think I figured it out and pushed a fix: f9ab494

You want to try that with your code?

Yep, that works for me! Thanks.

@JimmXinu
Copy link
Owner

@JimmXinu JimmXinu commented Nov 4, 2020

I'm not sure why the commit has a different number, but I did incorporate this: 6debdf5

@JimmXinu JimmXinu closed this Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants