Preferences / view state migration #6740

RaymondLim · 2014-02-03T17:19:09Z

No description provided.

…references to the new apis.

…open.

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

dangoor · 2014-02-04T18:59:50Z

@RaymondLim regarding a project scope, I'm not sure that we'd want one (a .brackets.state.json file in every project). On the one hand, it seems cleaner to do that than storing these project-level values in the user's global file. On the other hand, we'd basically be creating .brackets.state.json in every directory the user opens and the user would need to keep adding it to .gitignore and such.

For that reason, I suggest that we stick with just the single state.json file in the user's appdata. If we were getting fancy, we could have a Layer object that automatically pulls out the project-specific view state. If we didn't do that, we could still do the equivalent in the meantime... in other words, have the view state look something like this:

{
    "project": {
        "/path/to/project": {
            "project.files": [list of project files here]
        }
    }
}

Without a layer, you could basically just call getViewState("project") and then navigate the JS objects the normal way. With a layer, the project would be automatically set and calling getViewState("project.files") would return the correct value for the current project.

If you look at the path layer in PreferencesBase, you'll see that layers don't really do anything fancy other than look inside of the JSON data for a preference.

RaymondLim · 2014-02-06T02:48:53Z

@dangoor Do you have an example of using a layer object? I can create "project.files" key with the data hierarchy that you suggested, but I have to navigate the JS objects to set/get the files. And I also wonder how to call convertPreferences to migrate old preferences to the new pref key with your suggested structure. I know that I can provide new key as "user project.files" via my callback but still don't know how to tell the location. So I think we also need to handle the layer objects in convertPreferences function.

dangoor · 2014-02-06T17:15:09Z

Assuming there's a ProjectLayer that looks something like this (untested, adapted straight from PathLayer):

    function ProjectLayer() {
        this.projectPath = null;
    }

    ProjectLayer.prototype = {
        key: "project",

        get: function (data, id, context) {
            if (!data || !this.projectPath) {
                return;
            }

            if (data[this.projectPath] && data[this.projectPath][id]) {
                return data[this.projectPath][id];
            }
            return;
        },

        getPreferenceLocation: function (data, id, context) {
            if (!data || !this.projectPath) {
                return;
            }

            if (data[this.projectPath] && data[this.projectPath][id]) {
                return this.projectPath;
            }

            return;
        },

        set: function (data, id, value, context, layerID) {
            if (!layerID) {
                layerID = this.getPreferenceLocation(data, id, context);
            }

            if (!layerID) {
                return false;
            }

            var section = data[layerID];
            if (!section) {
                data[layerID] = section = {};
            }
            if (value === undefined) {
                delete section[id];
            } else {
                section[id] = value;
            }
            return true;
        },

        getKeys: function (data, context) {
            if (!data) {
                return;
            }

            return _.union.apply(null, _.map(_.values(data), _.keys));
        },

        setProjectPath: function (projectPath) {
            this.projectPath = projectPath;
        }
    }

You would instantiate that layer and add it to the "user" scope in the stateManager. Then, when the project changes you need to call setProjectPath on the layer with the path of the new project. Once you do that, calling stateManager.get("project.files") will automatically retrieve the files for that project. When you're saving project.files, you would call stateManager.set("project.files", (new value), { location: { scope: "user", layer: "project" } }) and it will be saved to the current project's set of data.

You're correct that convertPreferences knows nothing about this, so it would need to be extended in some way to support that.

I don't know if there's any conflict between our changes, but you might consider rebasing on top of dangoor/6578-hidden-editor-options because I did change a fair bit there. You decide, though... my work there wasn't focused on things that are likely to impact the view state work much so any conflicts shouldn't be too hard to resolve.

…failing test in WorkingSetView tests.

RaymondLim · 2014-02-07T17:46:55Z

test/spec/WorkingSetView-test.js

@@ -155,7 +155,7 @@ define(function (require, exports, module) {
            });
        });

-        it("should rebuild the ui from the model correctly", function () {
+        xit("should rebuild the ui from the model correctly", function () {


@dangoor I have to comment out this specific test since it's using localStorage.setItem("doLoadPreferences", true) and localStorage.removeItem("doLoadPreferences"), and I don't know how to convert them to the new preferences model.

This appears to be the only test that is using the doLoadPreferences mechanism. It looks like this test is verifying that the files in the working set automatically get re-added to the working set. You could add an option to SpecRunnerUtils.createTestWindowAndRun (something like options.viewState) that takes an object to be passed to the MemoryStorage when the stateManager is created... that's an easy way for a test to control the initial values on startup.

RaymondLim · 2014-02-08T20:28:43Z

@dangoor Thanks a lot for your guide in designing the new PathLayer. I've already tried and it does not work mainly due to the empty "data" in the "user" scope object. The first time the layer object is called to set a value, it checks for the existence of "project" key and then "project[folderPath]" key and bails out if any of them is missing. I did call setProjectPath, but it does not initialize the necessary keys. Maybe I need to do that in the PathLayer object when setProjectPath is called.

Update: I was able to fix the issue of bailing out when "project" key and then "project[folderPath]" key are missing. I modified getPreferenceLocation to prevent from bailing as follows.

        getPreferenceLocation: function (data, id, context) {
            if (!data || !this.projectPath) {
                return;
            }

            return this.projectPath;
        }

Now I have one new bigger issue. All view states are stored in the layer regardless of whether it belongs to a project or not.

dangoor · 2014-02-10T16:28:03Z

You should be able to call

stateManager.set("key", "value", {
   scope: "user",
   "layer": "project",
   "layerID": "/foo/bar/baz"
});

Or, you could call it without the layerID and have the ProjectLayer fill that in.

...but it appears that you can't because a minor change would need to be made to set. Specifically,

                if (layer) {
                    var wasSet = layer.set(this.data[layer.key], id, value, context, location.layerID);

That snippet of code should check to see if this.data[layer.key] is undefined and, if it is, it should set it to {}, providing the ability for set to properly create new layer settings.

Does that make sense?

RaymondLim · 2014-02-12T17:29:19Z

I'm done with all the conversion including two view settings using the project layer. So it's ready for the review. One exception is the commented test case that is using localStorage.

dangoor · 2014-02-12T20:14:33Z

src/LiveDevelopment/main.js

@@ -213,7 +212,7 @@ define(function main(require, exports, module) {
        } else {
            LiveDevelopment.hideHighlight();
        }
-        prefs.setValue("highlight", config.highlight);
+        PreferencesManager.setViewState("livedev.highlight", config.highlight);


Is this one view state or an actual pref? Given that it's coming from a menu item, I think this should probably be a preference.

Well, we need a good definition of what constitutes a pref and what is a pure view state. I talked to @peterflynn and his view seems to be different from you. He considers any settings that users do not want to share across machines to be a view setting regardless of whether the user can change it from the UI. We talked about three settings that users can change from Brackets UI; sorting order in the working set, case and regexp settings in Find bar and livedev highlighting. I was originally thinking all of them to be actual pref, but he convinced me to see them as view settings.

I had been thinking of this difference in terms of what someone might edit by hand, but I like that definition of Peter's because it's really simple. Given that, making it view state seems fine by me.

I guess one vote in favor of the definition being "what someone might edit by hand" is in #6815. That adds a "staticserver.port" pref. You might want to share that between machines, but maybe not. The main thing is that humans have a reason to change that value.

dangoor · 2014-02-12T21:00:15Z

I'm still going on the review (I'm at ProjectManager). The way DocumentManager is working with the ProjectLayer isn't really the way layers are intended to be used... the layer seemed to be adding more code when it should actually be simplifying things. It may be possible to actually just delete some code there, but I'll need to think about whether there are consequences.

Ideally, the project path is just set in ProjectManager and DocumentManager would not need to look up the project path but just set its state on {scope: "user", layer: "project"} and have the value get saved in the right place.

dangoor · 2014-02-13T17:33:45Z

@RaymondLim I just pushed a sample change to e7c4eb6 so that you can see what I mean. Here's how the ProjectLayer should work, given that the project path is set in ProjectManager.

When setting a view state value:

If there's no context given, the PreferencesSystem will look to see where the value is already stored. If it's not in the project layer, then the value will automatically land in the user scope.
If there is a context given, it will look there to see what scope to put the value in.
1. If the location has no layer specified, it'll go in the base of the scope
2. If the layer is set in the location to "project", then the ProjectLayer takes over. If there is no projectPath set, and no layerID set, then the project layer won't set the value.

When retrieving a value, it tries to find the most specific matching value by walking the scopes and the layers in those scopes. So, it starts with the user scope and the project layer to see if there's a value there. So, get("project.files") should automatically get the project.files value for the current project.

(Edit: the remaining files would need to be cleaned up like DocumentManager before I finish the rest of the review)

dangoor · 2014-02-13T17:39:07Z

I don't think the performance will be any worse than what we have now, given that we've been storing this data in a single key in localStorage... but I noticed that my file is 50k already and I don't have that many projects or working set files. It's just a lot more obvious how much data we're storing in here! At some point, we might find it necessary to create a new Storage object that stores this data more efficiently (possibly in IndexedDB with a separate key for each preferences key...)

I'll make a note on the card that in scenario testing we might want to test file switching performance.

RaymondLim · 2014-02-13T18:37:13Z

Thanks for the sample change to use the project layer the right way. I just pushed another commit with the necessary changes. And I know that the state.json file can be pretty big due to the UpdateInfo that we store there.

dangoor · 2014-02-13T20:06:48Z

For the deprecation warnings, I was thinking of utility function that is something like this:

displayedWarnings = {};

function deprecationWarning(message) {
if (displayedWarnings[message]) { return; }
var error;
try {
    deprecationWarning.doesNotExist();
} catch (e) {
    error = e;
}
console.warn(message);
console.warn(error.stack);
displayedWarnings[message] = true;
}

exports.deprecationWarning = deprecationWarning;

That displays each message just once and includes a stack which will help the user identify where the problem is. Python's deprecation warnings also let you strip off some stack frames so that you can point directly at the caller. If you wanted to do that, there's a library for generating stack traces that returns consistently formatted traces in an array. But that's just a nice-to-have.

dangoor · 2014-02-13T20:08:42Z

I think the deprecation warning utility function and doc comments for the ProjectLayer are all that's left here. I'll probably do a bit more testing, but then I think this is ready for merging and scenario testing.

RaymondLim · 2014-02-14T17:53:11Z

@dangoor Can you explain what the try block supposed to do? Especially, it's the deprecationWarning.doesNotExist();.

dangoor · 2014-02-15T01:43:33Z

@RaymondLim It's getting an exception object so that you can get the stack in order to print what caused the deprecation warning to be displayed. As far as I know, causing an exception to be raised is the only way to get a stack trace in JS.

RaymondLim · 2014-02-15T02:20:02Z

Got it. Thanks!

Instead of the try block, we can also explicitly get the stack with console.warn(new Error().stack);

Also, I have a question for you regarding of displaying each message once. You suggested to show the preference key or client ID with the message. If so, the message for each deprecated api call will be different. Or am I missing something for not to be spammy with deprecation messages?

Conflicts: src/preferences/PreferencesManager.js

@RaymondLim

This is following the pattern @RaymondLim established in #6740.

dangoor · 2014-02-19T13:24:36Z

@RaymondLim I missed that you had commented here. Sorry!

The general idea is that we want to show each distinct usage of the deprecated call so that the person who wrote that code can go and fix it, without repeatedly showing that same call.

…function.

RaymondLim · 2014-02-20T02:25:09Z

@dangoor Ready for re-review. I think I implemented the deprecation warning as the way you want, although I'm not using stack trace library that you suggested.

dangoor · 2014-02-20T02:36:29Z

@RaymondLim The new DeprecationWarning module looks great at first glance (two minor nits: it's 2014, and you don't need the /*jslint line).

I'll finish the review in the morning.

…ing.

…in jslint directive any more.

dangoor · 2014-02-20T14:34:31Z

src/preferences/PreferencesManager.js

+        // migrating the old preferences to the new model. So if this is called without 
+        // having _doNotCreate set to true, then the caller is using the old preferences model.
+        if (!_doNotCreate) {
+            DeprecationWarning.deprecationWarning("getPreferenceStorage is called with client ID '" + clientID + ",' use PreferencesManager.definePreference instead.");


If clientID is an object, this will just say getPreferenceStorage is called with client ID '[object Object]'.

dangoor · 2014-02-20T15:03:24Z

I fixed the nits, since they were tiny things. I'm going to file a followup bug about the disabled test, but otherwise I think this is good to go.

Preferences / view state migration

RaymondLim added 3 commits January 27, 2014 14:34

Add new apis for view state migration and convert the remaining old p…

4eee5ea

…references to the new apis.

Fix the hang issue when viewing a search result with state.json file …

6d859c8

…open.

Implement view states migration from the old preference model.

72658eb

RaymondLim assigned dangoor Feb 3, 2014

Add deprecation warnings for the old APIs.

5412fa1

RaymondLim added 2 commits February 6, 2014 22:44

Fix all broken integration tests with one exception -- excluding one …

1f5ac19

…failing test in WorkingSetView tests.

Add xit to fix JSLint error.

bd58ce5

RaymondLim reviewed Feb 7, 2014
View reviewed changes

RaymondLim added 2 commits February 10, 2014 23:00

Store working set files in a project layer.

d8783b8

Store project tree states in the project layer.

1648369

dangoor reviewed Feb 12, 2014
View reviewed changes

Making changes to review feedbacks.

1a12690

Use the project layer in the right way.

2979901

Merge remote-tracking branch 'origin' into rlim/view-state-migration

edc3da8

Conflicts: src/preferences/PreferencesManager.js

dangoor added a commit that referenced this pull request Feb 18, 2014

Automatically saves prefs on set unless new doNotSave flag is set.

616c5e0

This is following the pattern @RaymondLim established in #6740.

dangoor mentioned this pull request Feb 18, 2014

Automatically saves prefs on set unless new doNotSave flag is set. #6909

Merged

JSDoc for project layer object and implement new deprecation warning …

b51cef3

…function.

RaymondLim added 4 commits February 19, 2014 18:46

Remove "Error" messge from the stack.

bc1c935

Concatenate call stack to deprecation message and show it as one warn…

78f79fd

…ing.

Add "console" to globals directive since we don't set "devel" option …

1bd9196

…in jslint directive any more.

dangoor reviewed Feb 20, 2014
View reviewed changes

Fixes nits (including deprecation warning showing proper client ID)

6e68636

dangoor added a commit that referenced this pull request Feb 20, 2014

Merge pull request #6740 from adobe/rlim/view-state-migration

c00028b

Preferences / view state migration

dangoor merged commit c00028b into master Feb 20, 2014

dangoor deleted the rlim/view-state-migration branch February 20, 2014 15:07

marcelgerber mentioned this pull request Feb 20, 2014

Working set gone after view state migration #6934

Closed

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

[CLOSED] Preferences / view state migration brackets-archive/bracketsIssues#6117

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preferences / view state migration #6740

Preferences / view state migration #6740

RaymondLim commented Feb 3, 2014

dangoor commented Feb 4, 2014

RaymondLim commented Feb 6, 2014

dangoor commented Feb 6, 2014

RaymondLim Feb 7, 2014

dangoor Feb 13, 2014

RaymondLim commented Feb 8, 2014

dangoor commented Feb 10, 2014

RaymondLim commented Feb 12, 2014

dangoor Feb 12, 2014

RaymondLim Feb 12, 2014

dangoor Feb 13, 2014

dangoor Feb 14, 2014

dangoor commented Feb 12, 2014

dangoor commented Feb 13, 2014

dangoor commented Feb 13, 2014

RaymondLim commented Feb 13, 2014

dangoor commented Feb 13, 2014

dangoor commented Feb 13, 2014

RaymondLim commented Feb 14, 2014

dangoor commented Feb 15, 2014

RaymondLim commented Feb 15, 2014

dangoor commented Feb 19, 2014

RaymondLim commented Feb 20, 2014

dangoor commented Feb 20, 2014

dangoor Feb 20, 2014

dangoor commented Feb 20, 2014

Preferences / view state migration #6740

Preferences / view state migration #6740

Conversation

RaymondLim commented Feb 3, 2014

dangoor commented Feb 4, 2014

RaymondLim commented Feb 6, 2014

dangoor commented Feb 6, 2014

RaymondLim Feb 7, 2014

Choose a reason for hiding this comment

dangoor Feb 13, 2014

Choose a reason for hiding this comment

RaymondLim commented Feb 8, 2014

dangoor commented Feb 10, 2014

RaymondLim commented Feb 12, 2014

dangoor Feb 12, 2014

Choose a reason for hiding this comment

RaymondLim Feb 12, 2014

Choose a reason for hiding this comment

dangoor Feb 13, 2014

Choose a reason for hiding this comment

dangoor Feb 14, 2014

Choose a reason for hiding this comment

dangoor commented Feb 12, 2014

dangoor commented Feb 13, 2014

dangoor commented Feb 13, 2014

RaymondLim commented Feb 13, 2014

dangoor commented Feb 13, 2014

dangoor commented Feb 13, 2014

RaymondLim commented Feb 14, 2014

dangoor commented Feb 15, 2014

RaymondLim commented Feb 15, 2014

dangoor commented Feb 19, 2014

RaymondLim commented Feb 20, 2014

dangoor commented Feb 20, 2014

dangoor Feb 20, 2014

Choose a reason for hiding this comment

dangoor commented Feb 20, 2014