Zettlr icon indicating copy to clipboard operation
Zettlr copied to clipboard

Root Workspace Sort Dropdown (Alphabetical)

Open csjasonl opened this issue 3 years ago • 5 comments

Description

Added dropdown box in the workspace directory list to sort root workspaces alphabetically (ascending/descending). Addresses enhancement #3057 which was concerned with same-name root directories of different parent folders not being ordered alphabetically.

Image of the dropdown image

image

image

Changes

  • Added .ts file 'source/win-main/file-manager/util/sort-directories.ts' containing the sort function.
  • Modified .vue file 'source/win-main/file-manager/file-tree.vue' to incorporate GUI dropdown and function usage. small dropdown textbox is positioned next to the Workspaces header, and scales with the sidebar and window size.
  • Added unit test file 'test/sort-directories.spec.ts' to test function use cases. Runs alongside other unit tests.

Additional information

- The development team is doing this as part of a university course, and would appreciate any feedback

  • Translations are required for the new phrases: ** gui.sort_by_option ** gui.alphabetical_asc_option ** gui.alphabetical_desc_option placeholder text is available until translation files are updated
  • The default behaviour for ordering root directories is still available as the default "Sort By" option.
  • Other ordering types (e.g. by access or creation date) can be more easily implemented in future.
  • Layered parent same-name directories aren't shown, but they may still sorted (e.g. Directory A/X/Root & B/X/Root are shown as Root (X)).

Tested on: Windows 10 Home 64-bit (10.0 Build 19044), Ubuntu 20.04.4 LTS, macOS BigSur 11.6

csjasonl avatar May 18 '22 10:05 csjasonl

Thank you for opening your first PR! 🎉 We are very happy and would like to thank you very much for your contribution. If everything checks out, we'll make sure to review the PR as soon as possible and give feedback. In the meantime, to make the reviewing process as fast as possible, you can help us by checking the following things:

  • Did you follow the JSStandard coding style? - Did you comment everywhere where the necessity of a piece of code or the way it was implemented is not immediately obvious?
  • Did you attempt to stick as much to current coding habits as possible? (Note that this does not apply to pieces of code where we ourselves obviously violated good coding practices, which unfortunately happens sometimes. But please indicate this in your PR so that we know what you rectified!)

Furthermore, make sure that the linter does not complain, which will check your code on every new commit. If the linter task fails, make sure to run yarn lint locally and check the file eslint_report.htm which will tell you precisely what went wrong. Stay sharp, and thanks again!

boring-cyborg[bot] avatar May 18 '22 10:05 boring-cyborg[bot]

Thank you very much for this PR. Thus far, it looks good from a code perspective, but I think there are a few conceptual issues we might want to discuss before moving forward.

  1. Should the sort really only happen in the renderer, or wouldn't it be better to sort the paths directly in the configuration to begin with? That way, if we ever decide to display the root folders (this is a 100% certainty) we won't lose that ability just because we forget to implement your sorter.
  2. Should this really be done using a dropdown inside the workspaces header? On macOS that would be a no-go. Plus, chances are that people only would want to set this setting once, which would make the preferences a more suitable place for this setting.
    • 2a: If we choose to stick to the dropdown, I recommend utilizing the select Vue component in the common directory, which already implements native styling and which saves you from modifying the CSS too much
    • 2b: Having the default option as an "Don't sort" sounds bogus to me and I think this might confuse people, so we should choose another wording for that
  3. There is an Intl.compare function, which I think the file sorting algorithm of the FSAL utilizes. This way, we can make sure that even non-Western scripts are sorted appropriately. Maybe you can even utilize that sorter for your purposes. Would keep the code DRY and mitigate potential bugs. We can move that utility function outside of the FSAL, if necessary
  4. Lastly, I just remembered that I think the roots are already sorted to some degree, so this functionality here would potentially interfere with that. We need to check that.

@kyaso What are your thoughts on this?

nathanlesage avatar May 19 '22 18:05 nathanlesage

  1. [...] we won't lose that ability just because we forget to implement your sorter.

I have no particular opinion on point 1, but if the quoted part is a potential issue, I would agree with moving the sort to the configuration (@nathanlesage: is "configuration" in this context equivalent to the "store.state.fileTree"?)

  1. Should this really be done using a dropdown inside the workspaces header?

I would also say that the preferences might be a more suitable place for that.

  1. There is an Intl.compare function, which I think the file sorting algorithm of the FSAL utilizes. This way, we can make sure that even non-Western scripts are sorted appropriately.

I agree with this one.

  1. Lastly, I just remembered that I think the roots are already sorted to some degree, so this functionality here would potentially interfere with that. We need to check that.

Unfortunately, I'm not too familiar with that part of the codebase. But yeah, I guess it might be worth investigating.

kyaso avatar May 22 '22 11:05 kyaso

Yes, I meant to implement it in the config provider, or in the FSAL. One of those two places already implements some form of root sorting, I think it should be the FSAL.

nathanlesage avatar May 22 '22 12:05 nathanlesage

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Aug 13 '22 09:08 stale[bot]