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

Added ability to change zoom using Ctrl + Scroll in Document Viewer #10964

Merged
merged 7 commits into from
Mar 4, 2024
Merged

Added ability to change zoom using Ctrl + Scroll in Document Viewer #10964

merged 7 commits into from
Mar 4, 2024

Conversation

cardionaut
Copy link
Contributor

I have added the ability to change the zoom level using Ctrl + Scroll in the PDF Document Viewer.
Additionally, I have limited the zoom-out to ~1 page by setting a minWidth, as an unlimited zoom-out led to following problems:

  • Occasional display errors (e.g. no text displayed for certain pages)
  • No content can be seen if zoomed out too far, might be unclear to the user what happened and that zooming in will fix problem

The Document Viewer is still not very user friendly in my opinion and I am not sure how much further development time should be invested in its current state.
It might be a better option to get rid of it alltogether and always use the "Open file (F4)" option when a user wants to open a PDF.

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

cardionaut and others added 4 commits March 3, 2024 14:18
Observed problems with infinite zoom-out:
- Occasional display errors (e.g. no text displayed for certain pages)
- No content can be seen if zoomed out too far, might be unclear to user what happened and that zooming in will fix problem
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool change. Just one little code style nitpick. Then it's good to go in my eyes.

Siedlerchr
Siedlerchr previously approved these changes Mar 3, 2024
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 3, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@koppor
Copy link
Member

koppor commented Mar 4, 2024

I fully agree that the internal PDF viewer is a mess. The discussions are long ongoing. See https://sourceforge.net/p/jabref/feature-requests/667/ and some other feature request at https://sourceforge.net/p/jabref/discussion/318824/thread/4a6a1d4d/. It is more about convenience for the users not to have changing apps. For me, I would say: We should use existing apps, because we cannot implement a good PDF viewer on our own.

Maybe, we should investigate in a "proper" connection to existing PDF viewers. Similar to LaTeX editors do it.

I would first start with SumatraPDF - https://www.sumatrapdfreader.org/docs/Command-line-arguments. Then with Acrobat Reader? (https://stackoverflow.com/a/9554528/873282). I thought, Acrobat would have some specific communication infrastructure (DDX?), but I do not find it.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. One small nitpick (to reduce load on my side ^^)

CHANGELOG.md Outdated Show resolved Hide resolved
@koppor
Copy link
Member

koppor commented Mar 4, 2024

In case, we go to keep the viewer internally - we should check out

However, the most important part is to be able to highlight words and add text comments. Not sure, which framework will support that.

@koppor
Copy link
Member

koppor commented Mar 4, 2024

@cardionaut I just committed my CHANGELOG.md update to reduce load on your side ^^

@Siedlerchr Siedlerchr enabled auto-merge March 4, 2024 10:28
@cardionaut
Copy link
Contributor Author

@cardionaut I just committed my CHANGELOG.md update to reduce load on your side ^^

@koppor Thanks!
I am still new to this, thought that resolving would automatically apply the changes, sorry :)

@Siedlerchr Siedlerchr added this pull request to the merge queue Mar 4, 2024
Merged via the queue into JabRef:main with commit 84463e7 Mar 4, 2024
20 checks passed
@cardionaut cardionaut deleted the doc-viewer-scroll-to-zoom branch March 4, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants