Committing fix for Issue 1705

[sagarsane]

Hello,

Here is the pull request for fix for issue #1705
Added a switch case to set the dialogLabel correctly depending on which
Navigation option is chosen.

Thanks!
Sagar

[njx]

Thanks for submitting this. The issue with this fix is that the user can manually change the initial shortcut character later, and the dialog label would need to change to match in order to stay consistent.

[sagarsane]

Thanks @njx , yes, I thought of that too and it is perhaps not the best way to put a switch case around the prefix variable.

In that case, should there be a deeper check like a Navigate Menu Item click and bubble that all the way through to the dialogHTML and dialogLabel so that it stays consistent?

[peterflynn]

The other complication is that extensions can add additional search "modes," so ideally there should be some way for an extension to specify what label to use when its mode is active.

That doesn't seem too hard, though -- you need to update the label whenever currentPlugin changes anyway, so at that point you can just ask the plugin for its label.

[njx]

@sagarsane - I actually wouldn't hang it off the menu click handler, since it can happen even if the user just retypes the shortcut. There probably should be a central updateDialogLabel() method that gets called on both initialization and whenever there's a change event in the search field. That also implies that when creating the dialogHTML, you should create a <span> to contain the label, and then modify the contents of that span via jQuery in updateDialogLabel().

[sagarsane]

Thanks @peterflynn, @njx !

Hmm .. I think I understand what you are saying. I will try it out tomorrow and over the weekend.
Thanks for the insight!

[sagarsane]

Note: These are not new commits. Just updating this branch to the current brackets' master!
Thanks!
Sagar

[sagarsane]

Hello!

Continuing on @njx and @peterflynn's suggestions! I had a couple of follow-up questions.

Is there a plan to save/store the prefixes that are used, was wondering about this as they are hardcoded and passed right now? As mentioned above, an extension can add a search mode to the system, assuming that it may also have a prefix, it would be needed to store them.

In that case, I was thinking it could be stored as something like this:

{
"NAVIGATE_MENU_PAIRS"  : {
                                                  "CMD_QUICK_OPEN" : [ "Quick Open", "" ],
                                                  "CMD_GOTO_LINE" : [ "Go to Line", ":" ],
                                                  "CMD_GOTO_DEFINITION" : [ "Go to Definition", "@" ]
                                              }
}

This way, we could use a similar switch case but with these stored variables (may be in string.js ?) so that even if there are any updates/additions, it should not be a bigger issue.

Just wanted your thoughts on this!

Thanks!
Sagar

[njx]

Sorry for the slow response--we were out in Europe last week. I like the idea of making the prefixes be data-driven, but am not sure the right way to do it is to have a static structure storing everything. Ideally, it would probably be part of the API for registering a plugin, and perhaps it would make sense for the central QuickOpen code to actually parse the shortcuts out.

That all seems like a pretty major refactoring though. So for now, I'd suggest just hardcoding it for the purposes of your fix. If we decide to refactor out the shortcuts to make them more general in the future, or if we see a lot of people building different types of Quick Open plugins, we could generalize it at that point.

[sagarsane]

Thanks @njx .. no problem.. I saw your tweets :) ..
Hmm.. I get your point. I also realized that this will require a lot of refactoring!

Will go with hardcoding it for now then.
Thanks again!

[njx]

Hi @sagarsane -- I've opened a new pull request (#2298) that builds on your commits and implements the functionality we discussed above. (Your commits are part of the new pull request, so you'll still get credit for them.) Closing this pull request in favor of the new one.