Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Switch language / syntax mode of current document #6409

Merged
merged 34 commits into from
Jul 10, 2014

Conversation

peterflynn
Copy link
Member

This is an update of @JakeStoeffler's PR #4276. It's merged up with the latest master and addresses the main code review comments I made there. I also added some simple unit tests.

I still have two concerns about merging this immediately, though:

1) The dropdown UI doesn't look great. It uses a native <select>, which is just hard to style... and the way the statusbar is laid out makes it look misaligned in this case:
language picker

It would look nicer if we used an HTML popup like code hints, context menus, and the Recent Projects dropdown... but it would definitely add a bunch more work here. So maybe we can skate by with the current <select>-based UI for now. @larz0, any thoughts?

2) This foregrounds all the bugs in #2911 a lot more. I added a checklist of issues there. Are there any that are prominent enough to act as blockers here? (JS code hints seems like it might be the most glaring issue, but unfortunately that's one of the harder ones to fix too).

JakeStoeffler and others added 10 commits June 18, 2013 23:07
…rackets into pflynn/JakeStoeffler-languageselector-updated

# By Jake Stoeffler
# Via Jake Stoeffler
* 'languageselector' of https:/JakeStoeffler/brackets:
  Setting default language resets force flag
  Move forced flag to Document
  Replace modal dialog with custom <select>
  Fix API changes to not publicly expose things we shouldn't ;)
  Pass Strings by reference (per #4252)
  Add reset button; add file name to dialog title
  Initial commit of language select dialog

Conflicts:
	src/extensions/default/JSLint/thirdparty/jslint
- Disable language switching for untitled documents
- Remove unneeded imports
- Switch from deprecated CollectionUtils to lodash
- Fix label truncation bug
- Fix bug where language change is made redundantly multiple times
- Rename some methods for clarity
- Add two simple unit tests
@TomMalbran
Copy link
Contributor

The algiment issue is caused by the padding around the actual select. The select doesn't have any border, but it still starts where the "LESS" text is. We could fix it by removing the padding around the select and placing it into the select.

@lkcampbell
Copy link
Contributor

@peterflynn, @jasonsanjose, if you guys are in the process of figuring out the Sprint 37 pull requests to merge, this one gets my vote. This functionality would be very useful, especially if the document specific preference keep track of the language assigned to the document.

@njx
Copy link
Contributor

njx commented Feb 4, 2014

I added this to Nominations on the Kanban board.

@busykai
Copy link
Contributor

busykai commented Feb 27, 2014

@peterflynn, do you have time to merge the current master to this PR? I'll see how bad the issues from your checklist in #2911 look like... The first three seem to be quite important to address.

…ler-languageselector-updated

Conflicts:
	src/editor/EditorStatusBar.js
	src/widgets/StatusBar.html
@busykai
Copy link
Contributor

busykai commented Feb 28, 2014

@peterflynn, merged master into your branch locally. let me know if you want me to push it to your branch.

bad alignment can be solved by adding left padding and the same amount of negative margin to the select. i have this fix in my local branch as well.

@peterflynn
Copy link
Member Author

Sounds good -- thanks!

@TomMalbran
Copy link
Contributor

Instead of using a select can we use something similar to the button dropdown used in the File Filters and CSS Quick Edit, once the File filters are merged?

@peterflynn
Copy link
Member Author

@TomMalbran That's a great idea! Would probably make it easier to indicate the 'current' value too (e.g. placing a bullet or checkmark next to it as in menu-based picker UIs).

@TomMalbran
Copy link
Contributor

The design is nicer too and won't have the same problems as the select has now. The checkmarks will look good too. We just need to wait until the Find Filters are merged to be able to use it.

@busykai
Copy link
Contributor

busykai commented Mar 1, 2014

@TomMalbran great idea indeed! The native select looks out of the place, but I was struggling to find existing replacement. I'll fetch it once it lands.

@busykai
Copy link
Contributor

busykai commented Mar 7, 2014

OK, so #7015 has landed, I can take the widget from there now...

@peterflynn
Copy link
Member Author

Cool, yay!

@peterflynn
Copy link
Member Author

@busykai Awfully sorry for the delay. Thanks for all the fixes! And the unit test additions look great too.

So there's really just that one seeming bug in JavaScriptCodeHints/main.js, and then the two smaller questions/issues (whether to make Document.setLanguageOverride() private & understanding why the bit in setCurrentDocument() was moved in that last commit).

If we wanted to get this in for Sprint 0.41, we should try to land it by Friday. Do you have cycles this week to finish it off? If not lmk -- I can probably push up those last cleanups and then get someone else to review that incremental diff quickly.

@peterflynn
Copy link
Member Author

Oh, and we need to merge up with master too

@busykai
Copy link
Contributor

busykai commented Jun 19, 2014

@peterflynn I will have some today and tomorrow morning. Let's see if I can make it. I'll let you know once I run out of cycles.

…ler-languageselector-updated

Conflicts:
	src/styles/brackets_patterns_override.less
@busykai
Copy link
Contributor

busykai commented Jun 20, 2014

@peterflynn, ready. Didn't look into making Document.setLanguageOverride() private. Please feel free to make any changes necessary -- I won't be able to get back to this today.

@JeffryBooher JeffryBooher removed this from the Release 0.41 milestone Jun 20, 2014
@@ -623,6 +621,18 @@ 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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, were you seeing cases where the event was sent with previous === current? That shouldn't ever happen... (I don't think it blocks landing this, but if there's a bug in EditorManager it'd be good to know about it!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've verified this is impossible, and removed that check in cc47043

…r-languageselector-updated

* origin/master: (66 commits)
  updating 'Last translated' SHA to reflect last pull request
  Use XML highlighting for .xaml files
  Wordsmithing (@njx)
  Update Getting Started (How to change the project, why?)
  Update CM to the latest
  Easier notation
  Revert change to non-translation file
  Change Copyright year in strings.js
  Updated by ALF automation.
  Update Sha
  German translation (Sprint 41)
  modalbar already showing, adjust for old height
  Fixes after Review
  Tweak the Command constant for the new command which doesn't appear on the file menu.
  alf updates for release branch
  Changes after review.
  Small fixes
  Spanish translations for Release 0.41
  Updated by ALF automation.
  isolated fix for changing height of modal bar with filter picker
  ...
- Fix bug in DropdownButton that led to it often being positioned too far
to the left
- CSS to join more cleanly with top border of status bar (not perfect,
though: the dropshadow & animation both still feel a little odd)
@peterflynn
Copy link
Member Author

I've pushed up a quick tweak to improve how the dropdown/popup gets positioned. Whew! I think this is finally ready to roll now. @busykai Thanks so much for all your patience with my dilly-dallying on this :-( And @JakeStoeffler Thanks for originally getting the ball rolling all those months ago!

peterflynn added a commit that referenced this pull request Jul 10, 2014
…ctor-updated

Switch language / syntax mode of current document
@peterflynn peterflynn merged commit b409212 into master Jul 10, 2014
@peterflynn peterflynn deleted the pflynn/JakeStoeffler-languageselector-updated branch July 10, 2014 23:26
@peterflynn
Copy link
Member Author

@larz0 I don't think it's critical, but if you have time in the next week or so would you mind taking a look at the language switcher popup's appearance? The default dropshadow looks a little awkward here because it makes the popup feel separated from the status bar button below it (normally, this popup drops downward as seen in the Find in Files bar... but not here). Similarly, the dropdown opening animation seems calibrated for when it drops down instead of upward. Can you think of any easy tweaks that would improve that stuff?

@larz0
Copy link
Member

larz0 commented Jul 11, 2014

@peterflynn np I can take a look.

@codenamezjames
Copy link

Hey what is the status on this i was just thinking about making a extension to auto save the untitled doc to my temp folder so i could use highlighting. If this is coming soon i wont waste my time. Thanks for all the awesome work you guys are doing!

@sprintr
Copy link
Contributor

sprintr commented Dec 16, 2015

This has been merged a while ago. Secondly it doesn't deal with untitled files.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.