New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addresses issue #10846 #10850

Merged
merged 3 commits into from Apr 10, 2015

Conversation

Projects
None yet
4 participants
@thehogfather
Contributor

thehogfather commented Apr 8, 2015

@abose @peterflynn (apologies it isn't solely related to #10846 - it felt sensible to combine into one PR)
Fold states are now persisted using viewState (state.json)
Added copyright text into foldcode.js and foldgutter.js
Removed strings.js file
Removed latex-fold helper
Removed menu item for custom region folding
Disable custom region folding by default

Addresses issue #10846 - fold states are now persisted using viewStat…
…e (state.json)

Added copyright text into foldcode.js and foldgutter.js
Removed strings.js file
Removed latex-fold helper
Removed menu item for custom region folding
Disable custom region folding by default
@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose Apr 8, 2015

Contributor

Code folding is enabled in all files by default, and in some files like txt files it looks a bit odd.
Would it be a good thing to enable code folding in only HTML,XML,js Etc. and disabled in all other file types?

Contributor

abose commented Apr 8, 2015

Code folding is enabled in all files by default, and in some files like txt files it looks a bit odd.
Would it be a good thing to enable code folding in only HTML,XML,js Etc. and disabled in all other file types?

@thehogfather

This comment has been minimized.

Show comment
Hide comment
@thehogfather

thehogfather Apr 8, 2015

Contributor

@abose I suspect the fold marks are appearing in a txt file due to indentations. A suggestion would be to disable the alwaysUseIndentFold option so that only files with registered mode helpers for code-folding have fold markers?

Contributor

thehogfather commented Apr 8, 2015

@abose I suspect the fold marks are appearing in a txt file due to indentations. A suggestion would be to disable the alwaysUseIndentFold option so that only files with registered mode helpers for code-folding have fold markers?

Removed string for collapse custom region menu.
Set alwaysUseIndentFold preference to be false by default
Addresses #10848 - changes to preference files are now updated live in brackets without needing to restart
Show outdated Hide outdated src/nls/fi/strings.js
collapseKey = "Ctrl-Alt-[",
expandKey = "Ctrl-Alt-]",
collapseAllKey = "Alt-1",
expandAllKey = "Shift-Alt-1";

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

On master, I can't seem to override any of these shortcuts using keymap.json. I can't remember if this is some limitation of the user keybindings feature (e.g. can't override shortcuts defined by an extension? -- cc @RaymondLim)... But to users it will just feel like a bug since they don't know which things are implemented under the hood as a built-in extension.

If it's tricky to fix we should spin this off as a separate bug though.

@peterflynn

peterflynn Apr 9, 2015

Member

On master, I can't seem to override any of these shortcuts using keymap.json. I can't remember if this is some limitation of the user keybindings feature (e.g. can't override shortcuts defined by an extension? -- cc @RaymondLim)... But to users it will just feel like a bug since they don't know which things are implemented under the hood as a built-in extension.

If it's tricky to fix we should spin this off as a separate bug though.

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

Ack, scratch that -- I had the syntax backwards. It does work on master. We should just make sure it still works in this PR too with the new, more dynamic way bindings are registered & unregistered.

@peterflynn

peterflynn Apr 9, 2015

Member

Ack, scratch that -- I had the syntax backwards. It does work on master. We should just make sure it still works in this PR too with the new, more dynamic way bindings are registered & unregistered.

}
//create menus
if (!menuExists(codeFoldingMenuDivider)) {

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

I'm not clear on why we need to check all these individually. If one of them exists, they all should... But also, it seems like we could just maintain an internal flag like isInitialized to prevent doing everything in init() when it's already been done. (I'm assuming the core problem here is that preferences broadcasts change events sometimes when the pref value hasn't actually changed, which could lead to trying to initialize twice and thus duplicating all the menu items).

@peterflynn

peterflynn Apr 9, 2015

Member

I'm not clear on why we need to check all these individually. If one of them exists, they all should... But also, it seems like we could just maintain an internal flag like isInitialized to prevent doing everything in init() when it's already been done. (I'm assuming the core problem here is that preferences broadcasts change events sometimes when the pref value hasn't actually changed, which could lead to trying to initialize twice and thus duplicating all the menu items).

This comment has been minimized.

@thehogfather

thehogfather Apr 9, 2015

Contributor

@peterflynn I couldn't get it working properly for some reason without this smelly explicit conditions. But please feel free to have a go and update it.

@thehogfather

thehogfather Apr 9, 2015

Contributor

@peterflynn I couldn't get it working properly for some reason without this smelly explicit conditions. But please feel free to have a go and update it.

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

Looks like the divider gets recreated every time you set the "enabled" pref to true at runtime. That's kind of an edge case -- I'll see if I can address it in my PR instead of worrying about it here.

@peterflynn

peterflynn Apr 9, 2015

Member

Looks like the divider gets recreated every time you set the "enabled" pref to true at runtime. That's kind of an edge case -- I'll see if I can address it in my PR instead of worrying about it here.

This comment has been minimized.

@peterflynn

peterflynn Apr 10, 2015

Member

This part is fixed in my PR now

@peterflynn

peterflynn Apr 10, 2015

Member

This part is fixed in my PR now

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 9, 2015

Member

@thehogfather This looks great overall, thanks for posting this update so quickly! I added a few comments above.

One other note: this fixes bug #10848 for the "enabled" preference, but the other preferences still seem to need a restart or close/reopen file to take effect. Did you want to wait for another PR to fix those other prefs?

Member

peterflynn commented Apr 9, 2015

@thehogfather This looks great overall, thanks for posting this update so quickly! I added a few comments above.

One other note: this fixes bug #10848 for the "enabled" preference, but the other preferences still seem to need a restart or close/reopen file to take effect. Did you want to wait for another PR to fix those other prefs?

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 9, 2015

Member

@thehogfather Fyi, I've opened PR #10859 with some UI changes as well as a few code tweaks. Some of it duplicates change you've made here (I started working on it before you posted this), so once your PR is merged I'll rebase mine on top of your changes. But please take a look and see if there's anything else there you'd be concerned about.

Member

peterflynn commented Apr 9, 2015

@thehogfather Fyi, I've opened PR #10859 with some UI changes as well as a few code tweaks. Some of it duplicates change you've made here (I started working on it before you posted this), so once your PR is merged I'll rebase mine on top of your changes. But please take a look and see if there's anything else there you'd be concerned about.

@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose Apr 9, 2015

Contributor

@thehogfather Now that custom region fold support is removed, fold helper - region-fold.js could also be removed.

Contributor

abose commented Apr 9, 2015

@thehogfather Now that custom region fold support is removed, fold helper - region-fold.js could also be removed.

Moved preference strings from nls/lang to vars in Prefs.js
Changes to all preferences are now live
Removed region-fold and custom-region fold strings
@thehogfather

This comment has been minimized.

Show comment
Hide comment
@thehogfather

thehogfather Apr 9, 2015

Contributor

@abose @peterflynn I think this should be closer to ready than before. Cleaned up the strings and removed custom-region folding completely. Now over to you. Happy to help with anything else you direct my way.

Contributor

thehogfather commented Apr 9, 2015

@abose @peterflynn I think this should be closer to ready than before. Cleaned up the strings and removed custom-region folding completely. Now over to you. Happy to help with anything else you direct my way.

FADE_FOLD_BUTTONS = "Fade fold buttons",
FADE_FOLD_BUTTONS_HELP = "Hides the fold buttons unless the mouse is over the gutter",
MAX_FOLD_LEVEL = "Max fold level",
MAX_FOLD_LEVEL_HELP = "Used to limit the number of nested folds to find and collapse when View -> Collapse All is called or Alt is held down when collapsing. Should improve performance for large files.";

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

This feels a little weird -- might be better for these to just be doc comments on the definePreference() calls for now... but not a big deal. I think this would be ok to merge as-is.

@peterflynn

peterflynn Apr 9, 2015

Member

This feels a little weird -- might be better for these to just be doc comments on the definePreference() calls for now... but not a big deal. I think this would be ok to merge as-is.

@@ -273,6 +264,11 @@ define(function (require, exports, module) {
if (previous) {
saveLineFolds(previous);
}
} else {
if (current && current._codeMirror) {

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

Nit: can remove the & current._codeMirror here too

@peterflynn

peterflynn Apr 9, 2015

Member

Nit: can remove the & current._codeMirror here too

This comment has been minimized.

@peterflynn

peterflynn Apr 10, 2015

Member

Nm, this is taken care of in my PR

@peterflynn

peterflynn Apr 10, 2015

Member

Nm, this is taken care of in my PR

prefs.definePreference("maxFoldLevel", "number", 2,
{name: strings.MAX_FOLD_LEVEL, description: strings.MAX_FOLD_LEVEL_HELP});
{name: MAX_FOLD_LEVEL, description: MAX_FOLD_LEVEL_HELP});
prefs.definePreference("folds", "object", {});

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

This should be turned into PreferencesManager.stateManager.definePreference(...

@peterflynn

peterflynn Apr 9, 2015

Member

This should be turned into PreferencesManager.stateManager.definePreference(...

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 9, 2015

Member

Looks like minFoldSize and alwaysUseIndentFold still require close & reopen to take effect. Do you want to spin that off as a separate bug?

Member

peterflynn commented Apr 9, 2015

Looks like minFoldSize and alwaysUseIndentFold still require close & reopen to take effect. Do you want to spin that off as a separate bug?

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 9, 2015

Member

Hmm, it also looks like folds are no longer remembered when you close & reopen a file, even with a completely clean slate of prefs. Seems like something is broken with the view-state storage changes here...

Member

peterflynn commented Apr 9, 2015

Hmm, it also looks like folds are no longer remembered when you close & reopen a file, even with a completely clean slate of prefs. Seems like something is broken with the view-state storage changes here...

@thehogfather

This comment has been minimized.

Show comment
Hide comment
@thehogfather

thehogfather Apr 9, 2015

Contributor

WRT minFoldSize and alwaysUseIndentFold lets file a separate bug with steps to reproduce. When I test it I get the effects immediately within the brackets.json file being edited (after saving).

Contributor

thehogfather commented Apr 9, 2015

WRT minFoldSize and alwaysUseIndentFold lets file a separate bug with steps to reproduce. When I test it I get the effects immediately within the brackets.json file being edited (after saving).

@@ -85,8 +93,9 @@ define(function (require, exports, module) {
* @param {Object} folds the fold ranges to save for the current document
*/
function setFolds(path, folds) {
store[path] = simplify(folds);
prefs.set(foldsKey, store);
var allFolds = PreferencesManager.getViewState(foldsKey);

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

You need || {} here too -- that's why saving folds is broken currently.

@peterflynn

peterflynn Apr 9, 2015

Member

You need || {} here too -- that's why saving folds is broken currently.

This comment has been minimized.

@peterflynn

peterflynn Apr 9, 2015

Member

One other thought... rather than dumping folding state for all files together in one map, it might make sense to bucket it by project, like we do for other similar prefs like the scroll position & selection state of editors.

MainViewManager has an example of how to do that:

            context = { location : { scope: "user",
                                     layer: "project" } },
            state = PreferencesManager.getViewState(PREFS_NAME, context);

and

            context         = { location : { scope: "user",
                                         layer: "project",
                                         layerID: projectRoot.fullPath } },
        ...
        PreferencesManager.setViewState(PREFS_NAME, state, context);
@peterflynn

peterflynn Apr 9, 2015

Member

One other thought... rather than dumping folding state for all files together in one map, it might make sense to bucket it by project, like we do for other similar prefs like the scroll position & selection state of editors.

MainViewManager has an example of how to do that:

            context = { location : { scope: "user",
                                     layer: "project" } },
            state = PreferencesManager.getViewState(PREFS_NAME, context);

and

            context         = { location : { scope: "user",
                                         layer: "project",
                                         layerID: projectRoot.fullPath } },
        ...
        PreferencesManager.setViewState(PREFS_NAME, state, context);

This comment has been minimized.

@abose

abose Apr 10, 2015

Contributor

@peterflynn can you make the || {} change in your PR?
Also issues#10869 added for tracking the bucketing by project.

@abose

abose Apr 10, 2015

Contributor

@peterflynn can you make the || {} change in your PR?
Also issues#10869 added for tracking the bucketing by project.

This comment has been minimized.

@abose

abose Apr 10, 2015

Contributor

brackets/issues/10870 opened to track Code Folding preference minFoldSize and alwaysUseIndentFold require close & reopen of file to take effect.

@abose

abose Apr 10, 2015

Contributor

brackets/issues/10870 opened to track Code Folding preference minFoldSize and alwaysUseIndentFold require close & reopen of file to take effect.

This comment has been minimized.

@MarcelGerber

MarcelGerber Apr 10, 2015

Contributor

I think using the correct PreferencesManager.stateManager.definePreference(... will fix that, won't it?

@MarcelGerber

MarcelGerber Apr 10, 2015

Contributor

I think using the correct PreferencesManager.stateManager.definePreference(... will fix that, won't it?

This comment has been minimized.

@peterflynn

peterflynn Apr 10, 2015

Member

@MarcelGerber Yep, good point -- that should remove the need for || {} both here & in getFolds().

@peterflynn

peterflynn Apr 10, 2015

Member

@MarcelGerber Yep, good point -- that should remove the need for || {} both here & in getFolds().

@@ -317,29 +416,12 @@ define(function (require, exports, module) {
init();

This comment has been minimized.

@peterflynn

peterflynn Apr 10, 2015

Member

Btw, why do we need to call init() again on project open? Now that we're listening for preference changes at all times, it seems like this should be unnecessary?

@peterflynn

peterflynn Apr 10, 2015

Member

Btw, why do we need to call init() again on project open? Now that we're listening for preference changes at all times, it seems like this should be unnecessary?

This comment has been minimized.

@peterflynn

peterflynn Apr 10, 2015

Member

I removed this in my PR and things still seem to work ok

@peterflynn

peterflynn Apr 10, 2015

Member

I removed this in my PR and things still seem to work ok

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 10, 2015

Member

Btw, it's still a ways off but it will be interesting to think about what having the same document visible in two split panes will mean for code folding. Should collapsing a section in one pane automatically collapse it in the other? Seems like that would feel weird, but otoh if each editor has its own expand/collapse state for the same document, how will that be persisted? Which one "wins" if you close both views? Etc.

I'll add a note to that Trello story so we can think about it more when the time comes.

Member

peterflynn commented Apr 10, 2015

Btw, it's still a ways off but it will be interesting to think about what having the same document visible in two split panes will mean for code folding. Should collapsing a section in one pane automatically collapse it in the other? Seems like that would feel weird, but otoh if each editor has its own expand/collapse state for the same document, how will that be persisted? Which one "wins" if you close both views? Etc.

I'll add a note to that Trello story so we can think about it more when the time comes.

@peterflynn peterflynn added this to the Release 1.3 milestone Apr 10, 2015

abose added a commit that referenced this pull request Apr 10, 2015

@abose abose merged commit d36f263 into adobe:master Apr 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose Apr 10, 2015

Contributor

Loos good now; Merging this PR.
@peterflynn brackets/pull/10859 can now be merged.

Contributor

abose commented Apr 10, 2015

Loos good now; Merging this PR.
@peterflynn brackets/pull/10859 can now be merged.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 10, 2015

Contributor

@peterflynn It's the same issue for selections/cursors, having two states in one document is weird anyway.

Contributor

MarcelGerber commented Apr 10, 2015

@peterflynn It's the same issue for selections/cursors, having two states in one document is weird anyway.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 10, 2015

Member

@abose Wait, why did you merge this already? See my comments above -- #10850 (comment) and #10850 (comment) -- code folding persistence is completely broken in master now because of this PR. We should have waited before fixing that blocker before merging...

I'll file a high-priority bug to track it.

Member

peterflynn commented Apr 10, 2015

@abose Wait, why did you merge this already? See my comments above -- #10850 (comment) and #10850 (comment) -- code folding persistence is completely broken in master now because of this PR. We should have waited before fixing that blocker before merging...

I'll file a high-priority bug to track it.

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