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

Change "show file in explorer" to be different depending on the OS #370

Merged
merged 21 commits into from
Aug 31, 2024

Conversation

SupKittyMeow
Copy link
Contributor

Windows says explorer, Mac says Finder, and Linux just says filesystem (as I don't know what it is on Linux)

@CyanVoxel CyanVoxel added Type: UI/UX User interface and/or user experience Status: Review Needed A review of this is needed labels Aug 23, 2024
Copy link
Member

@CyanVoxel CyanVoxel left a comment

Choose a reason for hiding this comment

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

Adding to my Linux file manager comment, I would suggest putting the Linux case last in the if-else chain and instead have Windows as the middle case since both Windows and macOS have expected file explorer names, while for Linux + any other case we can use a generic catch-all term.

Also, are you able to use Ruff for linting or would you need some help with that?

@SupKittyMeow
Copy link
Contributor Author

Adding to my Linux file manager comment, I would suggest putting the Linux case last in the if-else chain and instead have Windows as the middle case since both Windows and macOS have expected file explorer names, while for Linux + any other case we can use a generic catch-all term.

Also, are you able to use Ruff for linting or would you need some help with that?

That should be fixed now. I haven't ever used Linux, but yeah I've heard that there's multiple file managers somewhere.

I've never actually heard of Ruff, so I probably need help on that, but I'll also look into it.

@CyanVoxel
Copy link
Member

In addition to the Ruff formatting it looks like MyPy is failing due to open_explorer_action being defined inside of the if-else blocks and the app run test is failing due to a mismatch between open_file_action and open_explorer_action

@CyanVoxel CyanVoxel added Status: Changes Requested Changes are quested to this Priority: Low Doesn't require immediate attention and removed Status: Review Needed A review of this is needed labels Aug 24, 2024
@SupKittyMeow
Copy link
Contributor Author

In addition to the Ruff formatting it looks like MyPy is failing due to open_explorer_action being defined inside of the if-else blocks and the app run test is failing due to a mismatch between open_file_action and open_explorer_action

Should be fixed now.

@SupKittyMeow SupKittyMeow requested a review from CyanVoxel August 25, 2024 02:26
@CyanVoxel
Copy link
Member

af4c0d1 is now trying to do a lot of additional things

@SupKittyMeow
Copy link
Contributor Author

af4c0d1 is now trying to do a lot of additional things

oh i did that because of all of the Ruff warnings

@CyanVoxel
Copy link
Member

oh i did that because of all of the Ruff warnings

Ruff provides additional lint suggestions in addition to what it's configured to format when run, either locally or though the workflow check. Using ruff format as described in the CONTRIBUTING.md would've fixed the issues the check was failing on. Now there are several additional changes that are out of the scope of this PR - I would just revert your changes made in af4c0d1 (please use more descriptive commit names than "thing" in the future) and run ruff format while leaving the other lint suggestions alone.

@SupKittyMeow
Copy link
Contributor Author

oh i did that because of all of the Ruff warnings

Ruff provides additional lint suggestions in addition to what it's configured to format when run, either locally or though the workflow check. Using ruff format as described in the CONTRIBUTING.md would've fixed the issues the check was failing on. Now there are several additional changes that are out of the scope of this PR - I would just revert your changes made in af4c0d1 (please use more descriptive commit names than "thing" in the future) and run ruff format while leaving the other lint suggestions alone.

Oh ok will do rn

SupKittyMeow and others added 2 commits August 24, 2024 22:35
@SupKittyMeow
Copy link
Contributor Author

Should be good now.

@CyanVoxel
Copy link
Member

I'll take a look at the checks failing, one moment

@SupKittyMeow
Copy link
Contributor Author

I forgot about something but now it should work.

Copy link
Member

@CyanVoxel CyanVoxel left a comment

Choose a reason for hiding this comment

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

Seems to be all good now! Thank you for working on this. I'll mark this as mergeable, but I'll still need to determine if this is better suited for the Alpha-v9.4 or post-#332 main branches. Nothing you need to worry about though - if I need to rebase I'll take care of that before pulling.

@CyanVoxel CyanVoxel added Status: Mergeable The code is ready to be merged System: Windows For Microsoft Windows System: Linux For Linux/BSD distributions System: macOS For the macOS operating system and removed Status: Changes Requested Changes are quested to this labels Aug 25, 2024
@CyanVoxel CyanVoxel changed the base branch from main to Alpha-v9.4 August 28, 2024 04:59
@CyanVoxel CyanVoxel merged commit dcb8ded into TagStudioDev:Alpha-v9.4 Aug 31, 2024
4 checks passed
CarterPillow pushed a commit to CarterPillow/TagStudio that referenced this pull request Sep 7, 2024
* Update item_thumb.py

* Update preview_panel.py

* Update video_player.py

* made it so preview_panel.py uses self.open_explorer_action instead of just normal open_explorer_action
CyanVoxel added a commit that referenced this pull request Sep 22, 2024
Ports the following thumbnail and related PRs from the `Alpha-v9.4` branch to `main` (v9.5+):
- (#273) Blender thumbnail support
- (#307) Add font thumbnail preview support
- (#331) refactor: move type constants to new media classes
- (#390) feat(ui): expanded thumbnail and preview features
- (#370) ui: "open in explorer" action follows os name
- (#373) feat(ui): preview support for source engine files
- (#274) Refactor video_player.py (Fix #270)
- (#430) feat(ui): show file creation/modified dates + restyle path label
- (#471) fix(ui): use default audio icon if ffmpeg is absent
- (#472) fix(ui): use birthtime for creation time on mac & win

Co-Authored-By: Ethnogeny <[email protected]>
Co-Authored-By: Theasacraft <[email protected]>
Co-Authored-By: SupKittyMeow <[email protected]>
Co-Authored-By: EJ Stinson <[email protected]>
Co-Authored-By: Sean Krueger <[email protected]>
CyanVoxel added a commit that referenced this pull request Oct 7, 2024
* feat: port v9.4 thumbnail + related feats to v9.5

Ports the following thumbnail and related PRs from the `Alpha-v9.4` branch to `main` (v9.5+):
- (#273) Blender thumbnail support
- (#307) Add font thumbnail preview support
- (#331) refactor: move type constants to new media classes
- (#390) feat(ui): expanded thumbnail and preview features
- (#370) ui: "open in explorer" action follows os name
- (#373) feat(ui): preview support for source engine files
- (#274) Refactor video_player.py (Fix #270)
- (#430) feat(ui): show file creation/modified dates + restyle path label
- (#471) fix(ui): use default audio icon if ffmpeg is absent
- (#472) fix(ui): use birthtime for creation time on mac & win

Co-Authored-By: Ethnogeny <[email protected]>
Co-Authored-By: Theasacraft <[email protected]>
Co-Authored-By: SupKittyMeow <[email protected]>
Co-Authored-By: EJ Stinson <[email protected]>
Co-Authored-By: Sean Krueger <[email protected]>

* remove vscode exceptions from `.gitignore`

* delete .vscode directory

* style: format for `ruff check`

* fix(tests): update `test_update_widgets_not_selected` test

* remove Send2Trash dependency

* refactor: use dataclass for MediaCateogry

* refactor: use enums for UI colors

* docs: add file docstring for silent_Popen

* refactor: replace logger with structlog

* use early return inside `ResourceManager.get()`

* add `is_ext_in_category()` method to `MediaCategory`

Add method to check if an extension is a member of a given MediaCategory.

* style: fix docstring style, missing type hints, rename `afm`

* fix: use structlog vars in logging

* refactor: move platform-dependent strings to PlatformStrings

* refactor: move `parents[2]` path to variable

* fix: undo logger regressions

---------

Co-authored-by: Ethnogeny <[email protected]>
Co-authored-by: Theasacraft <[email protected]>
Co-authored-by: SupKittyMeow <[email protected]>
Co-authored-by: EJ Stinson <[email protected]>
Co-authored-by: Sean Krueger <[email protected]>
@CyanVoxel CyanVoxel removed the Status: Mergeable The code is ready to be merged label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Doesn't require immediate attention System: Linux For Linux/BSD distributions System: macOS For the macOS operating system System: Windows For Microsoft Windows Type: UI/UX User interface and/or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants