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: Fixed crash that would occur when previewing PDFs in a RTL environment #12293

Merged
merged 34 commits into from
Jun 5, 2023

Conversation

Krytan
Copy link
Contributor

@Krytan Krytan commented May 6, 2023

image

In the picture you see the pdf priview in a RTL(Right To Left ) windows inviroment that's why the arrow start in the left side this is how people that speaks for example arabic, aramaic and hebrew see.
The bug was when viewing a PDF file using RTL and clicking in the left and right arrows the program would crash.

Validation
A proper test would be using a windows environment with RightToLeft direction
You can simulate RTL by going to pdfpreview.xaml And setting the FlowDirection to RightToLeft in FlipView and PipsPager
It should normally Crash if you don't use my fix version.

  1. Stand on a PDF file
  2. Have the preview panel open
  3. Click the arrows to move between pages
  4. Should work fine in both LTR and RTL inviroments

This should fix this bug #12037

@yaira2 yaira2 changed the title Fix: Fixed a crash when previewing a PDF in a RTL inviroment #12037 Fix: Fixed a crash when previewing a PDF in a RTL environment May 7, 2023
@Krytan
Copy link
Contributor Author

Krytan commented May 10, 2023

Useful information about the issue and changes: : The crash seems to be related to the default arrows in the flip-view not functioning properly when using a right-to-left FlowDirection. By default, the flowdirection is set to auto to support both left-to-right and right-to-left systems. However, the default arrows cannot be accessed or customized, so I created custom arrows that function similarly to the default arrows but with added functionality for right-to-left systems.

To test My changes, I changed the flowdirection from left-to-right (which works properly) to right-to-left (which was previously causing crashes), and it now works without crashing.

@Krytan
Copy link
Contributor Author

Krytan commented May 10, 2023

Note:

  1. Arrows are shown when the preview tab is open by default, in a pdf file
    before: they would only show if you hover the cursor over the pdf inside the preview tab.

  2. arrows layout is slight different from the default one.

Everything else should be the same, expect for it works for right to left systems now

@yaira2
Copy link
Member

yaira2 commented Jun 4, 2023

@Krytan can you confirm if this is a platform issue with the flip view? I think a cleaner solution would be to hardcode the flow direction for the flip view so that it's left to right.

@Krytan
Copy link
Contributor Author

Krytan commented Jun 5, 2023

@yaira2 The issue happens with windows 10 and 11 with the flipview as I tested on both platforms, interesting yea that should also fix the issue I gonna update the code.

@yaira2
Copy link
Member

yaira2 commented Jun 5, 2023

By platform issue I meant with the FlipView control rather than our own code.

@Krytan
Copy link
Contributor Author

Krytan commented Jun 5, 2023

oh aright yes that's correct since I could fix the issue by making a custom flipview rather than using the default flipview control buttons.

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 changed the title Fix: Fixed a crash when previewing a PDF in a RTL environment Fix: Fixed crash that would occur when previewing PDFs in a RTL environment Jun 5, 2023
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Jun 5, 2023
@yaira2 yaira2 merged commit 2a40abc into files-community:main Jun 5, 2023
@Krytan
Copy link
Contributor Author

Krytan commented Jun 4, 2024

@yaira2 Hello Yaira long time, can you assign me a new task I am looking to test my skills and continue contributing for files

@yaira2
Copy link
Member

yaira2 commented Jun 4, 2024

@Krytan thank you for reaching out, do you want to work on #8282?

@Krytan
Copy link
Contributor Author

Krytan commented Jun 4, 2024

@yaira2 Sure, it seems interesting thanks for quickly replying me back, I will try my best on the task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants