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: Adding service function RealTime Layout Service for cultures using RTL/LTR #16514

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

XTorLukas
Copy link
Contributor

@XTorLukas XTorLukas commented Nov 24, 2024

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

Microsoft issues

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

Important

This service function is only used to display the layout correctly.

  1. Opened Files
  2. Set RTL or LTR culture
  3. Automaticaly change flow direction layout

Tests for RTL

  • All Dialogs show correctly
  • All Tooltip show correctly
  • All Windows show correctly
  • All Drag position works correctly

@XTorLukas
Copy link
Contributor Author

fyi @0x5bfa

@XTorLukas XTorLukas marked this pull request as ready for review November 26, 2024 13:50
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Looks great. Does this also work for generated content dialogs such as the add new dialog that is shown when Ctrl+Shift+I entered?

src/Files.App/App.xaml.cs Outdated Show resolved Hide resolved
@@ -97,6 +100,9 @@ async Task ActivateAsync()
var userSettingsService = Ioc.Default.GetRequiredService<IUserSettingsService>();
var isLeaveAppRunning = userSettingsService.GeneralSettingsService.LeaveAppRunning;

Ioc.Default.GetRequiredService<IRealTimeLayoutService>().UpdateCulture(new(AppLanguageHelper.PreferredLanguage.Code));
MainWindow.Instance.InitializeContentLayout();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be called from MainWindow's constructor after InitializeComponent call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For calling, the culture must be already set by realTimeLayoutService.UpdateCulture

Copy link
Member

Choose a reason for hiding this comment

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

I mean InitializeContentLayout shouldn't be called in MainWindow's constructor as others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I understand, the generator works with the IoC as part of the initialization to implement the service. But the MainWindow constructor is called before the IoC is implemented.

src/Files.App/Data/Contracts/IRealTimeWindow.cs Outdated Show resolved Hide resolved
src/Files.App/Data/Contracts/IRealTimeWindow.cs Outdated Show resolved Hide resolved
@@ -7,7 +7,7 @@

namespace Files.App.Dialogs
{
public sealed partial class AddBranchDialog : ContentDialog, IDialog<AddBranchDialogViewModel>
public sealed partial class AddBranchDialog : ContentDialog, IDialog<AddBranchDialogViewModel>, IRealTimeControl
Copy link
Member

Choose a reason for hiding this comment

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

FYI, While this looks somewhat weird just for this but I'm planning to create an all-in-one service for ContentDialog & DynamicDialog to generate every dialog we use. The dialog contents will be Page class derivatives instead of the ContentDialog class.

For example:

IAppDialogService service = new();
var res = service.Show_AddBranchDialog();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IRealTimeControl can be used for all blocks that inherit from FrameworkElement, so also for Page.

Copy link
Member

Choose a reason for hiding this comment

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

nice 👍

@@ -263,63 +263,68 @@
</AnimatedIcon>
</ToggleButton>

<Button
x:Name="Back"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To correct arrow direction in RTL
image

Copy link
Member

Choose a reason for hiding this comment

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

@yaira2 fyi

Copy link
Member

Choose a reason for hiding this comment

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

What happens without this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The arrows are facing each other

Copy link
Member

Choose a reason for hiding this comment

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

Would it work to wrap these in a StackPanel (as you've done) and hardcode the FlowDirection of the StackPanel?

src/Files.App/UserControls/Sidebar/SidebarView.xaml.cs Outdated Show resolved Hide resolved
src/Files.App/ViewModels/Settings/GeneralViewModel.cs Outdated Show resolved Hide resolved
@XTorLukas
Copy link
Contributor Author

Does this also work for generated content dialogs such as the add new dialog that is shown when Ctrl+Shift+I entered?

Yes it works because it automatically inherits FlowDirection
I tested that too.
I've gone through all the windows that I know of.
But before merging it would be good to test in a wider collective so that the problematic windows can be traced.

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.

3 participants