Language switcher: connect to prefs & clean up some code #8444

peterflynn · 2014-07-17T21:39:56Z

Follow-on to PR #6409...

It may be easiest to review each commit individually, since they're fairly distinct:

Second commit - new feature:

Add "Set as Default" menu item to status bar language switcher -- adds a permanent file extension mapping to preferences, based on the current file's path-specific language override (if any) and its file extension.

First commit - cleanups:

Make LanguageManager the official public API that is used to set per-file overrides, rather than proxying through Document objects - remove Document.setLanguageOverride()
Clear path overrides when switching projects, so it's more obvious to users it's not something that gets persisted across sessions
Simplify how JS Code Hints listen for language updates: remove dependency on "currentDocumentLanguageChanged" event; remove unneeded extra 'if' tests

Third commit:

Improve "Set as Default" menu item -- gray it out when the current file has no path-specific override (i.e., when the current language already is the default for the current filetype)
Extend DropdownButton / DropdownEventHandler to support disabled items.
In the process, clean up dropdown keyboard navigation substantially -- fixing Page Up/Down work oddly in dropdowns without scrollbars #8433.

For clarity, I'd ideally like the "Set as Default" menu item to be grayed out when there is no path-specific override on the current file (i.e., when the current language already is the default for the current filetype)... but disabled items aren't a think in DropdownButton yet and I will need to do some cleanup there to get it working -- so I wanted to post this initial for review first. (this is now implemented)

* Make LanguageManager the official public API that is used to set per-file overrides, rather than proxying through Document objects - remove Document.setLanguageOverride() * Clear path overrides when switching projects, so it's more obvious that it's not something that gets persisted across sessions * Simplify how JS Code Hints listen for language updates: remove dependency on "currentDocumentLanguageChanged" event; remove unneeded extra 'if' tests

permanent file extension mapping to preferences, based on the current file's path-specific language override (if any) and its file extension.

peterflynn · 2014-07-17T21:42:14Z

src/extensions/default/JavaScriptCodeHints/main.js

@@ -624,18 +624,21 @@ define(function (require, exports, module) {
 * @param {Editor} previous - the previous editor context
 */
 function handleActiveEditorChange(event, current, previous) {
- // Uninstall "languageChanged" event listeners on the previous editor's document
- if (previous && previous !== current) {


previous !== current is guaranteed to be true for all "activeEditorChange" events, so I removed that check

…e doc's current language is already the default (i.e. there's no path-specific override). DropdownButton changes: * Support disabled items * Fix bug #8433 (Page Up/Down work oddly) - Only wrap around to other end of list when using arrow keys. Page Up/Down 'stick' at start/end of list instead (so they will act like Home/End when there's no scrollbar). * Add Home/End key support

redmunds · 2014-07-23T18:42:06Z

@peterflynn Is this something that should be merged for Release 0.42?

peterflynn · 2014-07-23T18:51:35Z

@redmunds Originally I was hoping so, but it seems like we're out of runway at this point...

peterflynn · 2014-07-23T18:52:20Z

The feature will be a little more confusing if left as-is for a sprint, but we can message that in the release notes & update notification

redmunds · 2014-07-23T20:03:41Z

Yeah, there's more changes than I thought -- best to wait on this one.

JeffryBooher · 2014-08-04T23:06:50Z

src/extensions/default/JavaScriptCodeHints/ScopeManager.js

@@ -1037,8 +1037,8 @@ define(function (require, exports, module) {
 * Do the work to initialize a code hinting session.
 *
 * @param {Session} session - the active hinting session


Session can't be null here either

Hmm, actually it looks entirely unused...

we should make that note in the doc and type it as optional

I'd rather not explicitly indicate it's ok to write code which passes null for that arg, unless we've verified it's correct for it to be ignoring the arg :-) And that seems outside the scope of this PR -- but I'll add a TODO to draw attention to it.

JeffryBooher · 2014-08-04T23:56:19Z

After changing the language of a custom file type (.jst) to "Javascript" then selecting "set as default for .jst files", the "Javascript" item in the droplist doesn't have (default) next to it. Seems like we want it to say "default" if we changed the default.

JeffryBooher · 2014-08-04T23:58:29Z

When selecting the item "Set as Default for files" (when enabled), nothing happens. The drop list should either disappear or the item should become disabled and the currently selected language should get the (default) next to it. Or we could just dismiss the droplist.

JeffryBooher · 2014-08-05T00:13:29Z

src/document/Document.js

- /**
- * Updates the language according to the file extension. If the current
- * language was forced (set manually by user), don't change it.
+ * Updates the language to match the current mapping given by LanguageManager


The original Language Switcher pr went through and we didn't update the events in Document.js -- specifically the LanguageChanged event (which is a past-tense event name and we normally use current-tense event names)

Actually it appears that there are NO events documented for document objects for some reason. is there a reason why we aren't documenting them? Are they documented somewhere else? I couldn't find them documented anywhere.

We also should notify lint extension authors that they should update their extensions to listen to the "languageChanged" event since most of them leave cruft behind after changing the language of a javascript file to "text". We might want to change it to "languageChange" to match the rest of the events if no one is using it yet as well.

The "languageChanged" event was added much earlier than the 0.42 language switcher PR -- it dates back to Sprint 21 (c00bfb1), so I'd rather not change it as part of this diff.

There are some events documented for Document (see docs on the constructor at the top of Document.js), but not this one. I'll add it.

peterflynn · 2014-08-05T09:18:30Z

@JeffryBooher I'm not seeing either of the behavior issues you described above:

Create a file named "test.foo"
Change language switcher from Text to JavaScript
Open language switcher again and choose "Set as Default for .foo Files"

The dropdown does close for me, and when I reopen it, it now shows "(default)" next to JavaScript instead of Text. Is there a different recipe I need to follow in order to repro the issues you saw?

* Use .hasClass() instead of .is() where possible * More succinct math operators in DropdownEventHandler._tryToSelect() * Document language-change events * Improve some more JS code hints docs

peterflynn · 2014-08-05T09:46:07Z

@JeffryBooher Changes pushed. Thanks for review!

JeffryBooher · 2014-08-05T15:16:29Z

@peterflynn It's probable that these are introduced by CEF 1750 and we should investigate them.

JeffryBooher · 2014-08-05T16:35:00Z

@peterflynn I went back to CEF 1547 and the problem still reproduces so it isn't 1750 specific.

Download and open the following project:
https://www.dropbox.com/s/2k1ak5lcktejnog/test1234.zip

open app.jst (language default should be text)
Select "Javascript" from the language drop list
Select "Set as default for .jst files"
==> Nothing happens

JeffryBooher · 2014-08-05T16:42:02Z

src/editor/EditorStatusBar.js

+
+ if (lang === LANGUAGE_SET_AS_DEFAULT) {
+ // Set file's current language in preferences as a file extension override (only enabled if not default already)
+ var fileExtensionMap = PreferencesManager.get("language.fileExtensions");


This is why I can't get this to work for me. You need to handle undefined as a return value from PreferenecesManager.get()

Uncaught TypeError: Cannot set property 'jst' of undefined EditorStatusBar.js:385

Yikes, good catch indeed,

@dangoor The docs for LanguageManager.definePreference() make it appear as if the initial value arg is required, so is the bug in this case that LanguageManager didn't set the default for "language.fileExtensions" to {}? (I found only a couple more cases throughout the codebase where a default isn't explicitly specified).

You're right. What LanguageManager does is less than ideal because _updateFromPrefs does:

var newMapping = PreferencesManager.get(pref) || {},

rather than just having a default set via definePreference.

mappings are set yet - don't leave default value of pref undefined. Clarify two other cases of definePreference() calls where we really *do* want the default value to be undefined by passing it explicitly. Also: add TODOs for unused Session arg in JS code hints.

peterflynn · 2014-08-06T06:38:49Z

@JeffryBooher Changes pushed to fix that bug & add TODOs in ScopeManager. Lmk if you need anything more before merging.

JeffryBooher · 2014-08-06T21:38:21Z

Looks good. All tests pass. The switcher behaves as it should for me now. Merging.

…ents Language switcher: connect to prefs & clean up some code

peterflynn · 2014-08-07T03:01:07Z

Thanks, Jeff!

peterflynn added 2 commits July 17, 2014 14:12

Add "Set as Default" menu item to status bar language switcher -- adds a

2f542eb

permanent file extension mapping to preferences, based on the current file's path-specific language override (if any) and its file extension.

peterflynn mentioned this pull request Jul 17, 2014

Switch language / syntax mode of current document #6409

Merged

peterflynn reviewed Jul 17, 2014
View reviewed changes

redmunds added the PR Triage Complete label Jul 31, 2014

JeffryBooher reviewed Aug 4, 2014
View reviewed changes

JeffryBooher reviewed Aug 5, 2014
View reviewed changes

Code review fixes:

23a3e91

* Use .hasClass() instead of .is() where possible * More succinct math operators in DropdownEventHandler._tryToSelect() * Document language-change events * Improve some more JS code hints docs

JeffryBooher reviewed Aug 5, 2014
View reviewed changes

redmunds assigned JeffryBooher Aug 5, 2014

JeffryBooher added a commit that referenced this pull request Aug 6, 2014

Merge pull request #8444 from adobe/pflynn/language-switcher-improvem…

90398b4

…ents Language switcher: connect to prefs & clean up some code

JeffryBooher merged commit 90398b4 into master Aug 6, 2014

JeffryBooher deleted the pflynn/language-switcher-improvements branch August 6, 2014 21:38

peterflynn mentioned this pull request Aug 7, 2014

Page Up/Down work oddly in dropdowns without scrollbars #8433

Closed

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Language switcher: connect to prefs & clean up some code brackets-archive/bracketsIssues#7545

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Language switcher: connect to prefs & clean up some code #8444

Language switcher: connect to prefs & clean up some code #8444

peterflynn commented Jul 17, 2014

peterflynn Jul 17, 2014

redmunds commented Jul 23, 2014

peterflynn commented Jul 23, 2014

peterflynn commented Jul 23, 2014

redmunds commented Jul 23, 2014

JeffryBooher Aug 4, 2014

peterflynn Aug 5, 2014

JeffryBooher Aug 5, 2014

peterflynn Aug 5, 2014

JeffryBooher commented Aug 4, 2014

JeffryBooher commented Aug 4, 2014

JeffryBooher Aug 5, 2014

peterflynn Aug 5, 2014

peterflynn commented Aug 5, 2014

peterflynn commented Aug 5, 2014

JeffryBooher commented Aug 5, 2014

JeffryBooher commented Aug 5, 2014

JeffryBooher Aug 5, 2014

peterflynn Aug 5, 2014

dangoor Aug 6, 2014

peterflynn commented Aug 6, 2014

JeffryBooher commented Aug 6, 2014

peterflynn commented Aug 7, 2014

Language switcher: connect to prefs & clean up some code #8444

Language switcher: connect to prefs & clean up some code #8444

Conversation

peterflynn commented Jul 17, 2014

Choose a reason for hiding this comment

redmunds commented Jul 23, 2014

peterflynn commented Jul 23, 2014

peterflynn commented Jul 23, 2014

redmunds commented Jul 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffryBooher commented Aug 4, 2014

JeffryBooher commented Aug 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Aug 5, 2014

peterflynn commented Aug 5, 2014

JeffryBooher commented Aug 5, 2014

JeffryBooher commented Aug 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Aug 6, 2014

JeffryBooher commented Aug 6, 2014

peterflynn commented Aug 7, 2014