Skip to content

Conversation

@Erquint
Copy link

@Erquint Erquint commented Nov 2, 2024

Added a ToggleSidebar action, imitating 4af2d59 as a template.

Resolved / Related Issues

Steps used to test these changes

  1. Launch Files.
  2. Take visual note of the sidebar, its size, presentation and border.
  3. Press [Ctrl]+[Alt]+[S] as the default keybinding for ToggleSidebar.
  4. Observe the sidebar and note if it has either collapsed or expanded.
  5. Repeat the keyboard input as in №3.
  6. Observe the sidebar and note if the opposite of №4 occured.
  7. Finally, repeat the input from №3 a third time to confirm that the toggle state machine survives a full loop onwards.
  8. Additionally, you may try to unbind the ToggleSidebar action and rebind it to another keyboard input to repeat the test with a non-default input.
  9. Could also test calling from the command palette ([Ctrl]+[Shift]+[P]).

Not quite seeing how I could test this myself yet. Assuming that having imitated an equivalent commit closely should make this a relatively straightforward contribution nevertheless.

As for the callback contents, it seemed like there's three ways of doing this — through:

Used the latter for this PR to closely match the reference commit.

@Erquint
Copy link
Author

Erquint commented Nov 2, 2024

Using the 4af2d59 commit as a template betrayed me in the localization strings department, heh.
Well, it was a trivial fix — the entries already existed for this. Just needed to reference them without creating duplicates.

@Erquint
Copy link
Author

Erquint commented Nov 2, 2024

All checks passed. Kept it a draft PR in accordance with your rules.

DO NOT submit a PR unless it the connected issue is marked as ready to build or approved indirectly by an org member. This enables us to have a discussion on the idea before anyone invests time on the implementation.

https://github.com/files-community/Files/blob/main/.github/CONTRIBUTING.md

Once the issue is marked as ready for PR, I'll make it a PR ready for review.

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.

Otherwise, LGTM.
However, note that this may be rejected after all because the linked issue is still marked as planning.

@Erquint
Copy link
Author

Erquint commented Nov 2, 2024

Thanks for the suggestion. And as for…

However, note that this may be rejected after all because the linked issue is still marked as planning.

That's why I'm keeping it a draft. 💁‍♂️

@Lamparter
Copy link
Contributor

I think you misunderstood.

Lamparter

This comment was marked as outdated.

@yaira2 yaira2 marked this pull request as ready for review November 3, 2024 01:27
@yaira2 yaira2 changed the title Feature: ToggleSidebar action. Feature: Added an action to collapse/expand the sidebar Nov 3, 2024
@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Nov 3, 2024
@yaira2
Copy link
Member

yaira2 commented Nov 3, 2024

  • Press [Ctrl]+[Alt]+[S] as the default keybinding for ToggleSidebar.
  • Observe the sidebar and note if it has either collapsed or expanded.

I've tried these steps, and it doesn't look like anything changes.

@Erquint
Copy link
Author

Erquint commented Nov 3, 2024

I'll see if I can build in CLI…
Not installing MVS under any circumstances.
Is there a way to make portable builds for rapid iteration without reinstallation?

@Lamparter
Copy link
Contributor

Wait, you didn't even test the changes 😬

And as for portable builds, you can download the AppxPackages archive generated from the CI pipeline.

@Erquint
Copy link
Author

Erquint commented Nov 3, 2024

I left the PR a draft, remember?
And included testing steps.
It's a collaborative project.

I did try the workflow artifact build under a sandbox but wasn't able to have it install.
Didn't want it to mess with my existing installation, but I guess I'm gonna get rid of Files anyway, so I'll replace it with a development/test build.
But waiting on GH actions to build the binaries might not make very rapid iteration of local development.

@Lamparter
Copy link
Contributor

Installing versions of Files from different production channels would not affect other channels.

@yaira2
Copy link
Member

yaira2 commented Nov 3, 2024

Didn't want it to mess with my existing installation, but I guess I'm gonna get rid of Files anyway, so I'll replace it with a development/test build.

The dev builds have a different package name so you can have both versions installed at the same time.

@yaira2
Copy link
Member

yaira2 commented Nov 3, 2024

I left the PR a draft, remember?
And included testing steps.
It's a collaborative project.

No harm done, don't worry about it 🙂

@yaira2 yaira2 marked this pull request as draft November 3, 2024 20:23
@Erquint
Copy link
Author

Erquint commented Nov 3, 2024

Installed the artifact with Visual Studio 2022 Developer PowerShell v17.11.5 bundled with MSBuildTools.
Normal PowerShell 7.4.6 couldn't install it, because M$ broke AppX in it.

Tested the action — it doesn't work.
Thinking if SidebarResizer_DoubleTapped can be helpful here.

private void SidebarResizer_DoubleTapped(object sender, DoubleTappedRoutedEventArgs e)
{
if (DisplayMode == SidebarDisplayMode.Expanded)
{
DisplayMode = SidebarDisplayMode.Compact;
e.Handled = true;
}
else
{
DisplayMode = SidebarDisplayMode.Expanded;
e.Handled = true;
}
}

DoubleTapped="SidebarResizer_DoubleTapped"

@Erquint
Copy link
Author

Erquint commented Nov 3, 2024

Erm, no. I did something stupid. Need to refer to the instance…

P. S.

Just spotted the typo — wasn't using the interface.

@Erquint
Copy link
Author

Erquint commented Nov 4, 2024

Wait, wasn't there an interface..? 🤔 I need rest…
Oh, it's the second instance of the same.

@Erquint
Copy link
Author

Erquint commented Nov 4, 2024

Yeah, I'm definitely doing this wrong.

private readonly ISidebarViewModel SidebarViewModel = Ioc.Default.GetRequiredService<ISidebarViewModel>();

Error: D:\a\Files\Files\src\Files.App\Actions\Show\ToggleSidebarAction.cs(9,20): error CS0246: The type or namespace name 'ISidebarViewModel' could not be found (are you missing a using directive or an assembly reference?) [D:\a\Files\Files\src\Files.App\Files.App.csproj]

SidebarViewModel.UpdateTabControlMargin();

Maybe I need to expand the interface… Erm, the type isn't found… Namespace scope..?
Please advise.

@0x5bfa
Copy link
Member

0x5bfa commented Nov 4, 2024

😂

@Erquint
Copy link
Author

Erquint commented Nov 4, 2024

@0x5bfa Well, that doesn't look like advice to me.

@Lamparter
Copy link
Contributor

Lamparter commented Nov 4, 2024

Why wouldn't you like to install Visual Studio?
I'm sure using it would make this easier.

@yaira2
Copy link
Member

yaira2 commented Nov 4, 2024

Lets try to stay on topic

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.

If you want advice.

@Erquint
Copy link
Author

Erquint commented Nov 4, 2024

Aight, let's try that.

Based on `SidebarDisplayMode.Expanded`/`.Compact`.

Co-authored-by: 0x5BFA <[email protected]>
@Erquint
Copy link
Author

Erquint commented Nov 4, 2024

ISidebarViewModel is still out of scope, so it's the same issue still.

ISidebarViewModel → SidebarViewModel.

Co-authored-by: 0x5BFA <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Pull requests that are ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Add action to collapse & expand sidebar

4 participants