Show the correct name after renaming a file #1116

Merged
merged 7 commits into from Sep 25, 2012

Conversation

Projects
None yet
3 participants
@skurfer
Owner

skurfer commented Sep 17, 2012

Fixes #1105. Not the cleanest fix, but there's a long comment to explain it.

Bonus fixes:

  • Don't return a new file (that clearly doesn't exist) if the rename fails.
  • Don't show the "Rename…" action if multiple files are selected. There was already code to prevent this, but validation wasn't turned on for the action.
@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 17, 2012

Should this be a notif as opposed to an alert? Just an idea…

Should this be a notif as opposed to an alert? Just an idea…

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Sep 18, 2012

Owner

Not opposed. That might be nicer as you don't have to explicitly dismiss it.

Owner

skurfer replied Sep 18, 2012

Not opposed. That might be nicer as you don't have to explicitly dismiss it.

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 17, 2012

How about returning the original file? Again, an idea which might not be the best one. Just throwing it out there :)

How about returning the original file? Again, an idea which might not be the best one. Just throwing it out there :)

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Sep 18, 2012

Owner

Well, I don't think we should both show an error and redisplay the interface, and I think we should show an error, so… :-)

Owner

skurfer replied Sep 18, 2012

Well, I don't think we should both show an error and redisplay the interface, and I think we should show an error, so… :-)

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 18, 2012

Yeah agreed.

Yeah agreed.

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 17, 2012

This should probably be tested on various systems with different languages set. Does this work, for example, for Japanese systems where label and name aren't necessarily the same?

This should probably be tested on various systems with different languages set. Does this work, for example, for Japanese systems where label and name aren't necessarily the same?

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Sep 18, 2012

Owner

Not sure. I had to lay it all out in a table to get it straight in my head. (FYI, GitHub's Markdown supports tables.)

On a system where the localized name matches the filesystem name:

action name label displayName
begin XYZ nil XYZ
rename to FOO FOO XYZ XYZ
set label nil FOO nil FOO

On a system where the localized name differs from the filesystem name:

action name label displayName
begin XYZ ABC ABC
rename to FOO FOO ABC ABC
set label nil FOO nil FOO

That's what you would want, right? For the name you just typed to appear in the interface? The only drawback I can see is that the object could live on for a long time (until a rescan) with no localized label. But is that a problem?

Other mitigating factors:

  • Files with localized names aren't the kind of thing you usually rename. (They probably belong to some application, and are not the things in ~/Documents.)
  • When you begin the rename process, the filesystem name is what appears in the third pane, so that's essentially what you're dealing with, and what should be displayed afterward.

So, it's fine …right? :-)

Owner

skurfer commented Sep 18, 2012

Not sure. I had to lay it all out in a table to get it straight in my head. (FYI, GitHub's Markdown supports tables.)

On a system where the localized name matches the filesystem name:

action name label displayName
begin XYZ nil XYZ
rename to FOO FOO XYZ XYZ
set label nil FOO nil FOO

On a system where the localized name differs from the filesystem name:

action name label displayName
begin XYZ ABC ABC
rename to FOO FOO ABC ABC
set label nil FOO nil FOO

That's what you would want, right? For the name you just typed to appear in the interface? The only drawback I can see is that the object could live on for a long time (until a rescan) with no localized label. But is that a problem?

Other mitigating factors:

  • Files with localized names aren't the kind of thing you usually rename. (They probably belong to some application, and are not the things in ~/Documents.)
  • When you begin the rename process, the filesystem name is what appears in the third pane, so that's essentially what you're dealing with, and what should be displayed afterward.

So, it's fine …right? :-)

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Sep 18, 2012

Owner

Changed the error to a notification. Much better.

Owner

skurfer commented Sep 18, 2012

Changed the error to a notification. Much better.

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 18, 2012

Owner

Great! :D

On 18 September 2012 19:29, Rob McBroom notifications@github.com wrote:

Changed the error to a notification. Much better.


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1116#issuecomment-8664999.

Owner

pjrobertson commented Sep 18, 2012

Great! :D

On 18 September 2012 19:29, Rob McBroom notifications@github.com wrote:

Changed the error to a notification. Much better.


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1116#issuecomment-8664999.

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Sep 20, 2012

Owner

The error notification needs to be localized, but I'm not really sure which .strings file to use. Do I need to create a new one for the plug-in? How do I tell the system to use it?

Owner

skurfer commented Sep 20, 2012

The error notification needs to be localized, but I'm not really sure which .strings file to use. Do I need to create a new one for the plug-in? How do I tell the system to use it?

@tiennou

This comment has been minimized.

Show comment Hide comment
@tiennou

tiennou Sep 20, 2012

Owner

You should use NSLocalizedStringFromTableInBundle, you can pass nil if you want that to go in Localizable.string, and the plugin's bundle as the bundle. Then you should be all set as soon as I get genstrings to work like I want it to.

Owner

tiennou commented Sep 20, 2012

You should use NSLocalizedStringFromTableInBundle, you can pass nil if you want that to go in Localizable.string, and the plugin's bundle as the bundle. Then you should be all set as soon as I get genstrings to work like I want it to.

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 25, 2012

Was it the desire to have the whole error message in the notification title and setting the message to nil. I know that things can get a little bit cluttered otherwise, so I'm just making sure it's what you wanted :)

Was it the desire to have the whole error message in the notification title and setting the message to nil. I know that things can get a little bit cluttered otherwise, so I'm just making sure it's what you wanted :)

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 25, 2012

Oh hang on, you forgot to localise the title, that's the problem ;-)

Oh hang on, you forgot to localise the title, that's the problem ;-)

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Sep 25, 2012

Owner

So I did. Fix on the way.

Owner

skurfer replied Sep 25, 2012

So I did. Fix on the way.

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Sep 25, 2012

Owner

Testing this out now.

Looks like you may have inadvertently prevented a crash by using a notif :)

Previously with the NSAlert:

NSAlert is being used from a background thread, which is not safe. This is probably going to crash sometimes. Break on _NSAlertWarnUnsafeBackgroundThreadUsage to debug. This will be logged only once. This may break in the future.

Other than the localisation of the title in the notif, works great :D

Owner

pjrobertson commented Sep 25, 2012

Testing this out now.

Looks like you may have inadvertently prevented a crash by using a notif :)

Previously with the NSAlert:

NSAlert is being used from a background thread, which is not safe. This is probably going to crash sometimes. Break on _NSAlertWarnUnsafeBackgroundThreadUsage to debug. This will be logged only once. This may break in the future.

Other than the localisation of the title in the notif, works great :D

pjrobertson added a commit that referenced this pull request Sep 25, 2012

Merge pull request #1116 from skurfer/issue1105
Show the correct name after renaming a file

@pjrobertson pjrobertson merged commit 73ebf51 into quicksilver:master Sep 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment