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

Feature : thumbnail zoom #195

Merged
merged 11 commits into from
Feb 5, 2018
Merged

Feature : thumbnail zoom #195

merged 11 commits into from
Feb 5, 2018

Conversation

mickaelalbertus
Copy link
Contributor

Feature #164 : allow the possibility to zoom for thumbnails (from 50 to 500 pixels).

@clefebvre
Copy link
Member

Hi @mickaelalbertus,

Thanks for implementing this.

Was the code written from scratch or ported from another project (evince, atril ...etc). If it was ported, can you add links to the original commits and bug report (if any) in the description?

@mickaelalbertus
Copy link
Contributor Author

It's written from scratch (I usually port from evince project but this functionality wasn't implemented).
However I report something interesting: the bug #139 is more visible. It happens when it's an icon view that is used (< 1500 pages about) and they reappeared when for instance we move the cursor is over the scrollbar so it's not a problem of long loading, it seems to be a problem of the gtk icon view. When I've time I will try to correct it!

@clefebvre
Copy link
Member

Thanks @mickaelalbertus, I'll try to review/merge as soon as possible. Things are a little slow after the release and because of the holidays :)

As a general comment, even if it doesn't apply here in the scope of this particular PR, do link to upstream commits/bug reports if you port features from other projects. It's nice to give credit first, but also for us, in the history, looking back at it later on, it's nice to understand why things were done and where they came from.

@clefebvre
Copy link
Member

Hi @mickaelalbertus

This is a pretty cool functionality. Here's my review, I think it's working well but it's missing a few things:

  • The generic thumbnail picture (the empty frame which is used to show the thumbnail before its picture is generated) isn't affected by the zoom. In other words, its size never changes, so when you zoom in or out and your zoom ratio isn't 1x, the size of generated thumbs isn't the same as the size of non-yet-generated thumbs (if that makes sense). This results in making the scrolledwindow jump during the generation of the thumbnail pictures and sometimes result in the view not rendering correctly.

  • The feature isn't discoverable. I would recommend to add zoom-in/out/reset buttons at the bottom of the thumb sidebar, maybe even a tooltip on the header to let users know they can Ctrl+mouse-wheel.

  • The feature isn't persistent. It would be nice to save the zoom level either specifically for the document or globally.

@mickaelalbertus
Copy link
Contributor Author

Hi, I totally agree with the two last point! For the first point I already think about it but I don't know how I can get the size of the thumbnail without loading it. Because the size of all thumbnails are not necessary the same?

@mickaelalbertus
Copy link
Contributor Author

Last point added: persistence of the thumbnails size

@mickaelalbertus
Copy link
Contributor Author

Here is the new interface for thumbnails sidebar:
image

@clefebvre
Copy link
Member

Hi @mickaelalbertus,

Please rebase and fix indentation/trailing-spaces.

@mickaelalbertus
Copy link
Contributor Author

Done!

@clefebvre
Copy link
Member

Thanks, new functions have a different indentation than old one, but I think that's ok. This looks good, I'm hoping to review/merge it early next week.

@clefebvre
Copy link
Member

@mickaelalbertus

image

Almost there. It works well.. point 1 is still an issue, but we can fix that later.

I'll fix the icons too. Great work overall 👍 :)

@clefebvre clefebvre merged commit 081831a into linuxmint:master Feb 5, 2018
@mickaelalbertus mickaelalbertus deleted the ThumbnailZoom branch March 31, 2018 07:58
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