Double click in the input config table to rename a config (+ bug fixes) #4960

uiryuu · 2024-05-29T12:29:35Z

I have read CONTRIBUTING.md

Description:
This PR:

Enable the user to change the config name by double clicking in the input config table
Refactors the configuration creation/duplication to eliminate duplicated code
Change some of the wordings to make things clearer
Stabilize the default input config files by using KeyValuePairs

Besides, this PR also fixes the usage of the Utility.quickAskPanel:

iina/iina/PrefKeyBindingViewController.swift

Lines 177 to 188 in 94176bb

    
           if Utility.quickAskPanel("config.file_existing", sheetWindow: self.view.window) { 
        
             // - delete file 
        
             do { 
        
               try fm.removeItem(atPath: newFilePath) 
        
             } catch { 
        
               Utility.showAlert("error_deleting_file", sheetWindow: self.view.window) 
        
               return 
        
             } 
        
           } else { 
        
             NSWorkspace.shared.activateFileViewerSelecting([URL(fileURLWithPath: newFilePath)]) 
        
             return 
        
           }

Previously it used the return value of the quickAskPanel, however, if the quickAskPanel is provided with the sheet window, it will always return false immediately without waiting for user input in the NSAlert panel. So 1) the "delete file" part of code could never be called, and 2) it always shows the existing file in Finder after the NSAlert is initiated (instead of after the user clicking the cancel button).

This PR changes the logic to show the existing file when clicking on OK, and do nothing when clicking on cancel. And also change the wording from "This should not happen..." to "A config file with the same filename already exists..."

I also remove the file deletion code because maybe its too dangerous to remove user files with "OK" or an enter hit in a seemingly unrelated alert window (the user want to create a config, but end up with deleting their file), let alone that part of code was never been executed before due to the erroneous usage.

- Eliminated duplicate code - Fix existing config file cannot be shown in Finder - Remove the ability to remove existing config file - Changed the alert text to reflect the changes

- Changed some wordings

- Make `userConfigNames` a computed property - Stablize the default input config files by using `KeyValuePairs`

svobs · 2024-05-30T04:58:18Z

When I saw the commit comment "don't maintain userConfigNames manually" I got excited. It would be better if Preference.Key.inputConfigs was no longer used at all, and instead the list of user configs was made by checking the input_conf directory for files ending in `.conf.

uiryuu · 2024-05-30T06:47:54Z

In the latest commit, I removed Preference.Key.inputConfigs and made userConfigs a computed variable, so that even if the user adds a file to the folder manually, the table will show the file when refreshed. This commit needs more testing.

access

iina/PrefKeyBindingViewController.swift

lhc70000 · 2024-05-30T22:02:51Z

iina/PrefKeyBindingViewController.swift

+        }
+      }
+      return nil
+    }


I also remove the file deletion code because maybe its too dangerous to remove user files with "OK" or an enter hit in a seemingly unrelated alert window (the user want to create a config, but end up with deleting their file), let alone that part of code was never been executed before due to the erroneous usage.

At least we should tell the user how to delete this file (where it is) in the dialog.

The default action (OK button or enter) will bring up the Finder and show the conflicting config file

iina/PrefKeyBindingViewController.swift

lhc70000 · 2024-05-30T22:13:58Z

iina/PrefKeyBindingViewController.swift

+
+  }
+
+  @IBAction func configFileListDoubleAction(_ sender: NSTableView) {


TBH I think when double clicking a table row people should expect the cell become editable, rather than a pop-up dialog. It's fine though.

Initially I wanted to implement this way, however, I noticed that if the renaming fails, it would be messy to bring up another dialog window to give the user info on why the action failed. So I just used the alert window as other actions (new, duplicate) in this window.

iina/PrefKeyBindingViewController.swift

lhc70000 · 2024-05-30T22:23:27Z

iina/PrefKeyBindingViewController.swift

@@ -434,11 +384,11 @@ extension PrefKeyBindingViewController: NSTableViewDelegate, NSTableViewDataSour
  }

  func tableViewSelectionDidChange(_ notification: Notification) {
+    guard !isLoading else { return }


Seems that isLoading's only usage is to filter out didChange notifications when calling loadConfigFile? If so, it's better to add a parameter like loadConfigFile(_, reloadTable:)

isLoading is preventing infinite loops, since we call loadConfigFile in tableViewSelectionDidChange, and we also call reloadData in loadConfigFile, which will cause another tableViewSelectionDidChange, thus causing an infinite loop.

svobs · 2024-05-31T05:29:59Z

In the latest commit, I removed Preference.Key.inputConfigs and made userConfigs a computed variable, so that even if the user adds a file to the folder manually, the table will show the file when refreshed. This commit needs more testing.

Very much appreciate the update! And yes @lhc70000 is right about the performance issue when checking the file system each time. IMHO it's still a good improvement to do a lazy property for now.

Later (or hopefully sooner) a good way to do a live update of the list would be to make a DispatchSourceFileSystemObject for the directory so we can get notified when its contents change, and do a reload in response to those. The author of #3724 posted some helpful links showing how to set this up. But it does take more than a little code to set up, so probably better to leave for later.

iina/PrefKeyBindingViewController.swift

low-batt

Pulled PR, built IINA, started IINA, opened settings. clicked on Key Bindings and the panel did not display any bindings for IINA Default:

If I click on another configuration the bindings are shown.

low-batt

I confirmed the latest commit fixes the issue with the bindings not being drawn when the panel first comes up.

I would think it would be better to follow the conventions of Finder where you press return and the name becomes editable. Finder also has to deal with the case where the file can't be renamed, like when you make the directory read only. Finder puts up an alert when that happens.

uiryuu · 2024-06-08T04:39:33Z

I would think it would be better to follow the conventions of Finder where you press return and the name becomes editable. Finder also has to deal with the case where the file can't be renamed, like when you make the directory read only. Finder puts up an alert when that happens.

We can make this happen in another PR

uiryuu added 2 commits May 29, 2024 20:22

Refactor config file creation/duplication logic +fixes/changes

af0d774

- Eliminated duplicate code - Fix existing config file cannot be shown in Finder - Remove the ability to remove existing config file - Changed the alert text to reflect the changes

Added double action for input config table to rename an entry

1499414

- Changed some wordings

uiryuu added feature: ui/ux type: refactor feature: preferences labels May 29, 2024

uiryuu requested a review from low-batt May 29, 2024 12:29

uiryuu force-pushed the input-config-table-fixes branch from 7208cac to 3765a80 Compare May 29, 2024 13:39

Refactor: don't maintain userConfigNames manually

feea96d

- Make `userConfigNames` a computed property - Stablize the default input config files by using `KeyValuePairs`

uiryuu force-pushed the input-config-table-fixes branch from 3765a80 to feea96d Compare May 29, 2024 13:46

Refactor: Remove Preference.Key.inputConfigs; compute userConfigs upon

8fb983c

access

uiryuu force-pushed the input-config-table-fixes branch from 24eed5c to 8fb983c Compare May 30, 2024 06:53

Refactor: Further simplify the logic

51ccba9

lhc70000 requested changes May 30, 2024

View reviewed changes

uiryuu added 3 commits May 31, 2024 10:22

Update based on reivew comments

6ee2a9d

Only read folder contents when loading a config file

8361488

Only load config file if the name is different than the currnet

7700bbd

svobs reviewed May 31, 2024

View reviewed changes

iina/PrefKeyBindingViewController.swift Show resolved Hide resolved

Edited based on review

60e8027

uiryuu requested a review from lhc70000 June 7, 2024 07:27

low-batt requested changes Jun 8, 2024

View reviewed changes

Fixed an issue where the keybinding mappings are not shown

ea1fa37

uiryuu requested a review from low-batt June 8, 2024 03:47

low-batt approved these changes Jun 8, 2024

View reviewed changes

lhc70000 approved these changes Jun 8, 2024

View reviewed changes

lhc70000 merged commit bb2d6c6 into develop Jun 8, 2024

lhc70000 deleted the input-config-table-fixes branch June 8, 2024 22:21

	if Utility.quickAskPanel("config.file_existing", sheetWindow: self.view.window) {
	// - delete file
	do {
	try fm.removeItem(atPath: newFilePath)
	} catch {
	Utility.showAlert("error_deleting_file", sheetWindow: self.view.window)
	return
	}
	} else {
	NSWorkspace.shared.activateFileViewerSelecting([URL(fileURLWithPath: newFilePath)])
	return
	}


		}

		@IBAction func configFileListDoubleAction(_ sender: NSTableView) {

Uh oh!

Double click in the input config table to rename a config (+ bug fixes) #4960

Double click in the input config table to rename a config (+ bug fixes) #4960

Uh oh!

Conversation

uiryuu commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svobs commented May 30, 2024

Uh oh!

uiryuu commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhc70000 May 30, 2024

Choose a reason for hiding this comment

Uh oh!

uiryuu May 31, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhc70000 May 30, 2024

Choose a reason for hiding this comment

Uh oh!

uiryuu May 31, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lhc70000 May 30, 2024

Choose a reason for hiding this comment

Uh oh!

uiryuu May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svobs commented May 31, 2024

Uh oh!

Uh oh!

low-batt left a comment

Choose a reason for hiding this comment

Uh oh!

low-batt left a comment

Choose a reason for hiding this comment

Uh oh!

uiryuu commented Jun 8, 2024

Uh oh!

Uh oh!

uiryuu commented May 29, 2024 •

edited

Loading

uiryuu commented May 30, 2024 •

edited

Loading

uiryuu May 31, 2024 •

edited

Loading