search

files-community / Files

A modern file manager that helps users organize their files and folders.
https://files.community
MIT License
34.9k stars 2.21k forks source link

Feature: Added an action to collapse/expand the sidebar #16422

Closed Erquint closed 1 month ago

Erquint commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

Erquint commented 1 month ago

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 commented 1 month ago

I think you misunderstood.

yaira2 commented 1 month ago
  • 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

yaira2 commented 1 month ago

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 commented 1 month ago

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

No harm done, don't worry about it 🙂

Erquint commented 1 month ago

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.
https://github.com/files-community/Files/blob/39df64d692a83f01eb69dab96dc261a3d0bc9976/src/Files.App/UserControls/Sidebar/SidebarView.xaml.cs#L193-L205 https://github.com/files-community/Files/blob/39df64d692a83f01eb69dab96dc261a3d0bc9976/src/Files.App/UserControls/Sidebar/SidebarView.xaml#L109

Erquint commented 1 month ago

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

P. S.

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

Erquint commented 1 month ago

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

Erquint commented 1 month ago

Yeah, I'm definitely doing this wrong.

https://github.com/files-community/Files/blob/95c21898e656a12a2aef363491537d584ae9c1fd/src/Files.App/Actions/Show/ToggleSidebarAction.cs#L9

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]

https://github.com/files-community/Files/blob/95c21898e656a12a2aef363491537d584ae9c1fd/src/Files.App/Actions/Show/ToggleSidebarAction.cs#L31

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

0x5bfa commented 1 month ago

😂

Erquint commented 1 month ago

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

Lamparter commented 1 month ago

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

yaira2 commented 1 month ago

Lets try to stay on topic

Erquint commented 1 month ago

Aight, let's try that.

Erquint commented 1 month ago

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