[CLOSED] Preferences / view state migration

[core-ai-bot]

Issue by RaymondLim Monday Feb 03, 2014 at 17:19 GMT Originally opened as https://github.com/adobe/brackets/pull/6740

---

---

RaymondLim included the following code: https://github.com/adobe/brackets/pull/6740/commits

[core-ai-bot]

Comment by dangoor Tuesday Feb 04, 2014 at 18:59 GMT

---

@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.

[core-ai-bot]

Comment by RaymondLim Thursday Feb 06, 2014 at 02:48 GMT

---

@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.

[core-ai-bot]

Comment by dangoor Thursday Feb 06, 2014 at 17:15 GMT

---

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.

[core-ai-bot]

Comment by RaymondLim Saturday Feb 08, 2014 at 20:28 GMT

---

@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.

[core-ai-bot]

Comment by dangoor Monday Feb 10, 2014 at 16:28 GMT

---

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?

[core-ai-bot]

Comment by RaymondLim Wednesday Feb 12, 2014 at 17:29 GMT

---

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.

[core-ai-bot]

Comment by dangoor Wednesday Feb 12, 2014 at 21:00 GMT

---

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.

[core-ai-bot]

Comment by dangoor Thursday Feb 13, 2014 at 17:33 GMT

---

@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:

1. 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.
2. 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)

[core-ai-bot]

Comment by dangoor Thursday Feb 13, 2014 at 17:39 GMT

---

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.

[core-ai-bot]

Comment by RaymondLim Thursday Feb 13, 2014 at 18:37 GMT

---

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.

[core-ai-bot]

Comment by dangoor Thursday Feb 13, 2014 at 20:06 GMT

---

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.

[core-ai-bot]

Comment by dangoor Thursday Feb 13, 2014 at 20:08 GMT

---

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.

[core-ai-bot]

Comment by RaymondLim Friday Feb 14, 2014 at 17:53 GMT

---

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

[core-ai-bot]

Comment by dangoor Saturday Feb 15, 2014 at 01:43 GMT

---

@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.

[core-ai-bot]

Comment by RaymondLim Saturday Feb 15, 2014 at 02:20 GMT

---

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?

[core-ai-bot]

Comment by dangoor Wednesday Feb 19, 2014 at 13:24 GMT

---

@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.

[core-ai-bot]

Comment by RaymondLim Thursday Feb 20, 2014 at 02:25 GMT

---

@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.

[core-ai-bot]

Comment by dangoor Thursday Feb 20, 2014 at 02:36 GMT

---

@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.

[core-ai-bot]

Comment by dangoor Thursday Feb 20, 2014 at 15:03 GMT

---

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.