yasirkula / UnitySimpleFileBrowser

A uGUI based runtime file browser for Unity 3D (draggable and resizable)
MIT License
817 stars 111 forks source link

Use conventional modifier key for macOS/iOS: Command instead of Control; Change Shift modifier behaviour. #76

Closed JoeyTeng closed 1 year ago

JoeyTeng commented 1 year ago
  1. 781d76a changes the shift key behaviour: it selects all items from previous selected to current, thus all items are selected in sequence. Previously, the current selected items is added to the selected paths list first.
  2. 73c7106 uses command key on macOS/iOS for multiple selection or select all, which is more conventional.
yasirkula commented 1 year ago

I can't find Keyboard.current.actionKey in the documentation and it isn't recognized on Input System 1.2.0. Perhaps it was just added in a very recent release of Input System?

JoeyTeng commented 1 year ago

Sorry I think I was searching the wrong documentation here. Let me change the code so to conditionally uses leftCommandKey and rightCommandKey when it is running on Apple devices.

In my testing, it runs perfectly so I guess it is actually running with Input.GetKey

JoeyTeng commented 1 year ago

By the way, you may want to reformat the code as it seems that the code is not automatically formatted in a good way in my end. Sorry I am not sure how to fix this as I have very new to C# and Unity (started my first Unity project a week ago)

yasirkula commented 1 year ago

Thank you for the recent changes. It looks good to me. However, to make the code clearer, I was thinking if we could separate the CTRL/Command key check logic into a separate private function (below CheckDirectoryWriteAccess function). Something like this:

private bool IsCTRLKeyHeld()
{
#if using new input system
#if mac
    check left and right command keys
#else
    check ctrl key
#endif
#else
#if mac
    check left and right command keys
#else
    check left and right ctrl keys
#endif
#endif
}

(Here, mac condition should be UNITY_EDITOR_OSX || ( !UNITY_EDITOR && UNITY_STANDALONE_OSX ))

This would make CTRL queries in the code much cleaner in my opinion.

JoeyTeng commented 1 year ago

Really good suggestion! I just implemented this n d5c24b5. Thank you for your code reviews and your great plugin 🥳

JoeyTeng commented 1 year ago

Hi @yasirkula , may I know if there is anything I should do before you merging this PR?

yasirkula commented 1 year ago

No I don't expect any further action from you. I've a busy schedule for the time being so I'm waiting for a suitable time.