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

Add “Show Current File in Finder” menu item (#5001) #5005

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ShlomoCode
Copy link
Contributor

@ShlomoCode ShlomoCode commented Jun 21, 2024


Description:
Adds a menu bar item (under File) to reveal the current file in Finder.
For some reason I was unable to set a Cmd+R shortcut, even though I set it through the interface builder, the shortcut doesn't work, so the PR does not currently include a shortcut.
Built and tested with XCode 15.1, on MacOS 14.5.

@low-batt
Copy link
Contributor

@svobs Should be able to explain how to add the shortcut for the menu item.

@uiryuu
Copy link
Member

uiryuu commented Jun 21, 2024

I think adding the keybinding in "IINA Default" input config and it can be automatically matched and show up in the menu.

Copy link
Member

@uiryuu uiryuu left a comment

Choose a reason for hiding this comment

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

Apart from what @low-batt has indicated, I have nothing to say. We should consider adding this into the default configuration, and we can add after merging this PR. The problem is should we add this to all default configs (mpv, VLC, Movist)?

iina/MainMenuActions.swift Show resolved Hide resolved
iina/Base.lproj/KeyBinding.strings Outdated Show resolved Hide resolved
@ShlomoCode ShlomoCode force-pushed the reveal-file branch 2 times, most recently from 54b5e93 to 1e8c04a Compare June 21, 2024 04:08
@ShlomoCode ShlomoCode changed the title Add “Reveal in Finder” menu item (closes #5001) Add “Show in Finder” menu item (closes #5001) Jun 21, 2024
@low-batt
Copy link
Contributor

We do need to get the menu shortcut working. The one suggested is ⌘R. I checked Mac keyboard shortcuts and this is how Apple uses that key:

Command-R: (1) When an alias is selected in the Finder: show the original file for the selected alias. (2) In some apps, such as Calendar or Safari, refresh or reload the page. (3) In Software Update, check for software updates again.

I don't think it is unreasonable to use ⌘R, but I'm not that familiar with IINA's key binding code. Need to hear from @svobs and @lhc70000.

@ShlomoCode ShlomoCode force-pushed the reveal-file branch 2 times, most recently from 6aa33e3 to 784f781 Compare June 21, 2024 04:49
@ShlomoCode ShlomoCode requested a review from uiryuu June 21, 2024 04:56
Copy link
Member

@uiryuu uiryuu left a comment

Choose a reason for hiding this comment

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

I'm okay with the changes. I'll try to abstract the logic of @IBAction func contextMenuShowInFinder(_ sender: NSMenuItem) after merging this PR. Not sure whether or not that's worthwhile

@svobs
Copy link
Contributor

svobs commented Jun 21, 2024

@svobs Should be able to explain how to add the shortcut for the menu item.

Add the 2 lines below to the case statement of handleIINACommand, in PlayerWindowController.swift:

internal func handleIINACommand(_ cmd: IINACommand) {
    switch cmd {
    // […]
    case .showCurrentFileInFinder:
      menuActionHandler.menuShowCurrentFileInFinder(.dummy)
    // […]

@svobs
Copy link
Contributor

svobs commented Jun 21, 2024

Having connectivity problems here...strange... After adding the code I posted above, everything will be in place for the end user to add a row like this one to your Key Bindings configuration:
Key column: Meta+r
Action column: #@iina show-current-file-in-finder

@ShlomoCode
Copy link
Contributor Author

I confirm that it is now possible to set a keyboard shortcut in the settings and it works.
CleanShot 2024-06-21 at 14 18 08@2x

One thing I noticed that when I use two screens MacOS opens the Finder on the built-in screen and does not focus on it, meaning the focus remains on the IINA window. When I only use the built-in screen it doesn't happen. But I'm guessing this is a macOS bug and not something we should be dealing with.

@svobs
Copy link
Contributor

svobs commented Jun 21, 2024

One thing I noticed that when I use two screens MacOS opens the Finder on the built-in screen and does not focus on it, meaning the focus remains on the IINA window. When I only use the built-in screen it doesn't happen.

I believe it has something to do with the Assign To setting in the Dock. Try setting this to different values for Finder and for IINA, respectively.
SCR-20240621-kvod

Some combinations appear to focus on the new Finder window and some do not. For me, using Desktop on Display 1 (main display) for Finder and All Desktops for IINA correctly changed the focus. So I suspect when some combinations don't give focus to the Finder window, that is by design for some reason.

I haven't been able to find documentation for this feature. From playing around, it looks like the All Desktops option will open windows for the app in the same screen that the mouse is currently in at the time of open. But I can't figure out what the None option is supposed to do. And I have no idea why sometimes multiple options are checked. If anyone has any info, please share :)

@low-batt low-batt linked an issue Jun 22, 2024 that may be closed by this pull request
@low-batt
Copy link
Contributor

The shortcut key still needs to be added to iina-default-input.conf, yes? I've not looked to see if that key is available in the other key bindings.

@svobs
Copy link
Contributor

svobs commented Jun 22, 2024

Add the 2 lines below to the case statement of handleIINACommand, in PlayerWindowController.swift

I was in a little bit of a hurry when I wrote that, but I found time to re-evaluate and have determined that adding a case to handleIINACommand is no longer needed. There is a newer mechanism introduced by the fix to #4567 which makes it mostly obsolete. Although it shouldn't hurt anything to leave it there, it can be removed.

I filed a PR, #5012, which has more info if curious.

@ShlomoCode
Copy link
Contributor Author

I'm not sure that Cmd+R fits here, because it's usually used to refresh (in browsers, XCode, etc.)
Might we want it for a streaming refresh? Thoughts?

@low-batt
Copy link
Contributor

I too am concerned about the choice of key.

From Spotlight keyboard shortcuts on Mac:

See a file in an app or the Finder
Command-R or Command-Double-click

So we have an Apple example of ⌘R being used to bring up Finder.

I need to defer to @svobs and @lhc70000 on the choice of key.

@ShlomoCode
Copy link
Contributor Author

I believe it has something to do with the Assign To setting in the Dock. Try setting this to different values for Finder and for IINA, respectively.

I use an external screen, not space (macOS virtual screen). It seems to only work for spaces, it doesn't show up for me for the external screen, only when I create two spaces on the same physical screen.
CleanShot 2024-06-23 at 02 32 52@2x
macOS 14.5

@saagarjha
Copy link
Member

I do want to note that this is feature already pretty much available because we present a document proxy icon, which makes it easy to locate the file and open a Finder window for it. I’m not opposed to also adding a “Reveal in Finder” menu item but it definitely should not be squatting on shortcut prime real estate like ⌘R by default.

@ShlomoCode ShlomoCode changed the title Add “Show in Finder” menu item (closes #5001) Add “Show Current File in Finder” menu item (closes #5001) Jun 23, 2024
@ShlomoCode ShlomoCode changed the title Add “Show Current File in Finder” menu item (closes #5001) Add “Show Current File in Finder” menu item (#5001) Jun 23, 2024
@svobs
Copy link
Contributor

svobs commented Jun 24, 2024

I do want to note that this is feature already pretty much available because we present a document proxy icon, which makes it easy to locate the file and open a Finder window for it.

The document icon is not available in legacy full screen or in music mode. It is lost when either the titled window style is removed or the window has titleVisibility = .hidden. I have bent over backwards trying to figure out how to add it back in manually as one can do with the other title buttons but can't seem to make it work, and can't find documentation anywhere.

@svobs
Copy link
Contributor

svobs commented Jun 24, 2024

Also agree that ⌘R is probably not the best default. I would associate it more with Reload or Refresh or some other command starting with R, particularly since the action is no longer called "Reveal in Finder". But I wouldn't use anything with S or F either as those have much stronger existing associations. Maybe just add another modifier key? ⇧⌘R? Looking at the existing IINA defaults, there doesn't seem to be a strong pattern to follow for which modifier(s) to add.

@low-batt
Copy link
Contributor

low-batt commented Jun 25, 2024

Apple guidance on custom keyboard shortcuts can be found here. Don't be confused by the entries in the Recommended usage column that are in the wrong row. It indicates the ⌘ key is preferred:

Prefer the Command key as the main modifier key in a custom keyboard shortcut.

Focusing on the keys that trigger IINA actions we can see there isn't a consistent pattern but many use ⇧⌘:
keys

So ⇧⌘ and one of the keys not already used seems right.

@uiryuu
Copy link
Member

uiryuu commented Jun 25, 2024

Not meant to divert the discussion too much, but there's an alternative is to not set any default keybindings for that and let the user to set whatever they want.

@low-batt
Copy link
Contributor

I thought about that, but configuring key bindings is a bit advanced. I would not be surprised if many users looked at the new menu item, saw there was no keyboard shortcut and gave up. Therefore I think it is important we provide a default key binding. I'm thinking ⌃⌘R due to the use of ⌘R by Spotlight.

@lhc70000
Copy link
Member

I suggest ⇧⌘R.

We use ⇧⌘ when possible and I prefer it over ⌃⌘ because it's more comfortable when pressing. The reason we have other modifiers for some shortcuts are

  • ⌘3, ⌘+, ⌘- for adjusting window size: these are major functions and should have corresponding mpv commands, but we have custom commands because they need extra handling
  • ⌃⌘P for PIP: ⇧⌘P is occupied (show playlist)
  • ⌥⌘M for music mode: TBH I couldn't remember 😢

@lhc70000 lhc70000 self-assigned this Jun 28, 2024
@low-batt
Copy link
Contributor

I agree, ⇧⌘R.

@low-batt
Copy link
Contributor

We also use the ⇧⌘ convention for the mpv defaults as well, but it looks like we switched to ⌥⌘ for Movist and VLC. So ⌥⌘R for Moviest and VLC?

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.

add Show in Finder menu command
6 participants