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

Commit

Permalink
Add new apis for view state migration and convert the remaining old p…
Browse files Browse the repository at this point in the history 
…references to the new apis.
  • Loading branch information
@RaymondLim
RaymondLim committed Jan 27, 2014
1 parent e1c3b88 commit 4eee5ea
Show file tree
Hide file tree
Showing 11 changed files with 101 additions and 111 deletions.
22 changes: 16 additions & 6 deletions src/LiveDevelopment/main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ define(function main(require, exports, module) {
ExtensionUtils = require("utils/ExtensionUtils"),
StringUtils = require("utils/StringUtils");

var prefs;
var params = new UrlParams();
var config = {
experimental: false, // enable experimental features
Expand Down Expand Up @@ -125,8 +124,8 @@ define(function main(require, exports, module) {
if (LiveDevelopment.status >= LiveDevelopment.STATUS_CONNECTING) {
LiveDevelopment.close();
} else {
if (!params.get("skipLiveDevelopmentInfo") && !prefs.getValue("afterFirstLaunch")) {
prefs.setValue("afterFirstLaunch", "true");
if (!params.get("skipLiveDevelopmentInfo") && !PreferencesManager.get("afterFirstLaunch")) {
PreferencesManager.setValueAndSave("afterFirstLaunch", "true");

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

This should be saved in the separate view state PreferencesSystem, not in the main prefs. As written here, the user's brackets.json file (in app data) will have an afterFirstLaunch key in it, which is just noise because no user will want to hand edit that.

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

Ahh, I see... this is not first launch of Brackets, this is first live dev launch. So this should be a bit of prefixed view state.

This comment has been minimized.

Sign in to view

Copy link
@RaymondLim

RaymondLim Feb 3, 2014

Author Contributor

Agree that this needs to be prefixed.
Dialogs.showModalDialog(
DefaultDialogs.DIALOG_ID_INFO,
Strings.LIVE_DEVELOPMENT_INFO_TITLE,
Expand Down Expand Up @@ -213,7 +212,7 @@ define(function main(require, exports, module) {
} else {
LiveDevelopment.hideHighlight();
}
prefs.setValue("highlight", config.highlight);
PreferencesManager.setValueAndSave("highlight", config.highlight);
}

/** Setup window references to useful LiveDevelopment modules */
Expand Down Expand Up @@ -256,9 +255,20 @@ define(function main(require, exports, module) {
});

// init prefs
prefs = PreferencesManager.getPreferenceStorage(module, {highlight: true});
PreferencesManager.definePreference("highlight", "boolean", true);

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

This should be defined in a prefixed preferences system... "highlight" is too generic, so we'd probably want something like "livedev.highlight". (See CodeInspection for an example of use of a prefixed system)

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

Also, this one is a bit more complicated. Live Development would likely want to know what the highlight setting is for the file that it is currently previewing. There isn't a good API for that yet, and that's what I'm working on now.

This comment has been minimized.

Sign in to view

Copy link
@RaymondLim

RaymondLim Feb 3, 2014

Author Contributor

Agree that this needs to be prefixed.

config.highlight = prefs.getValue("highlight");
/**
* @private
*
* Manage the conversion from old-style localStorage prefs to the new file-based ones.
*/
function _convertPreferences() {
PreferencesManager.convertPreferences(module, {"highlight": "user"});
}

_convertPreferences();

config.highlight = PreferencesManager.get("highlight");

// init commands
CommandManager.register(Strings.CMD_LIVE_FILE_PREVIEW, Commands.FILE_LIVE_FILE_PREVIEW, _handleGoLiveCommand);
Expand Down
7 changes: 3 additions & 4 deletions src/brackets.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,10 @@ define(function (require, exports, module) {
// the samples folder on first launch), open it automatically. (We explicitly check for the
// samples folder in case this is the first time we're launching Brackets after upgrading from
// an old version that might not have set the "afterFirstLaunch" pref.)
var prefs = PreferencesManager.getPreferenceStorage(module),
deferred = new $.Deferred();
var deferred = new $.Deferred();

if (!params.get("skipSampleProjectLoad") && !prefs.getValue("afterFirstLaunch")) {
prefs.setValue("afterFirstLaunch", "true");
if (!params.get("skipSampleProjectLoad") && !PreferencesManager.getViewState("afterFirstLaunch")) {

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

When making the top level API in PreferencesManager, I kind of assumed that most extensions would just be using prefs and not view state. If that's actually true, we may just want to expose the view state preferences system directly rather than exposing wrapper functions.

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

oh, I see. You just exposed those two functions (getViewState and setViewState) and the stateManager itself. That seems fine.
PreferencesManager.setViewState("afterFirstLaunch", "true");
if (ProjectManager.isWelcomeProjectPath(initialProjectPath)) {
FileSystem.resolve(initialProjectPath + "index.html", function (err, file) {
if (!err) {
Expand Down
13 changes: 2 additions & 11 deletions src/document/DocumentManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,6 @@ define(function (require, exports, module) {
*/
var _currentDocument = null;

/**
* @private
* @type {PreferenceStorage}
*/
var _prefs = {};

/**
* Returns the Document that is currently open in the editor UI. May be null.
* When this changes, DocumentManager dispatches a "currentDocumentChange" event. The current
Expand Down Expand Up @@ -844,7 +838,7 @@ define(function (require, exports, module) {
});

// append file root to make file list unique for each project
_prefs.setValue("files_" + projectRoot.fullPath, files);
PreferencesManager.setViewState("files_" + projectRoot.fullPath, files);

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

should we use . as a separator rather than _?

This comment has been minimized.

Sign in to view

Copy link
@RaymondLim

RaymondLim Jan 30, 2014

Author Contributor

I wonder this may cause a conflict with . also used for the current directory in a file path. This may not be a proble for projectRoot path since we won't have a project root path with ., but it may cause a conflict in other prefixed key for other file-path-based preferences.

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 31, 2014

Contributor

Actually, I wonder why it's done this way at all? Why not something like:

projectFiles[fullPath] = files;
PreferencesManager.setViewState("project.files", projectFiles);

If a user has a lot of different projects that Brackets knows about, this can really enlarge the view state... that's not a new issue, but it's something we should be aware of.

This comment has been minimized.

Sign in to view

Copy link
@RaymondLim

RaymondLim Feb 3, 2014

Author Contributor

Not sure I understand how to set up "projectFiles[fullPath]". What do we initialize to "projectFiles" here? But I can see that this view setting should be stored in the project scope by creating a ".state.json" file in fullPath. Can you tell me how to add a project scope? What is the api that I can call with fullPath as the argument?
}

/**
Expand All @@ -854,7 +848,7 @@ define(function (require, exports, module) {
function _projectOpen(e) {
// file root is appended for each project
var projectRoot = ProjectManager.getProjectRoot(),
files = _prefs.getValue("files_" + projectRoot.fullPath);
files = PreferencesManager.getViewState("files_" + projectRoot.fullPath);

console.assert(Object.keys(_openDocuments).length === 0); // no files leftover from prev proj

Expand Down Expand Up @@ -1028,9 +1022,6 @@ define(function (require, exports, module) {
exports.notifyPathNameChanged = notifyPathNameChanged;
exports.notifyPathDeleted = notifyPathDeleted;

// Setup preferences
_prefs = PreferencesManager.getPreferenceStorage(module);

// Performance measurements
PerfUtils.createPerfMeasurement("DOCUMENT_MANAGER_GET_DOCUMENT_FOR_PATH", "DocumentManager.getDocumentForPath()");

Expand Down
10 changes: 4 additions & 6 deletions src/extensions/default/RecentProjects/main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ define(function (require, exports, module) {

var KeyboardPrefs = JSON.parse(require("text!keyboard.json"));

var prefs = PreferencesManager.getPreferenceStorage(module);

/** @const {string} Recent Projects commands ID */
var TOGGLE_DROPDOWN = "recentProjects.toggle";

Expand All @@ -66,7 +64,7 @@ define(function (require, exports, module) {
* Warning: unlike most paths in Brackets, these lack a trailing "/"
*/
function getRecentProjects() {
var recentProjects = prefs.getValue("recentProjects") || [],
var recentProjects = PreferencesManager.getViewState("recentProjects") || [],
i;

for (i = 0; i < recentProjects.length; i++) {
Expand All @@ -91,7 +89,7 @@ define(function (require, exports, module) {
if (recentProjects.length > MAX_PROJECTS) {
recentProjects = recentProjects.slice(0, MAX_PROJECTS);
}
prefs.setValue("recentProjects", recentProjects);
PreferencesManager.setViewState("recentProjects", recentProjects);
}

/**
Expand Down Expand Up @@ -131,7 +129,7 @@ define(function (require, exports, module) {
newProjects.push(recentProjects[i]);
}
}
prefs.setValue("recentProjects", newProjects);
PreferencesManager.setViewState("recentProjects", newProjects);
$(this).closest("li").remove();
checkHovers(e.pageX, e.pageY);

Expand Down Expand Up @@ -195,7 +193,7 @@ define(function (require, exports, module) {

// remove project
recentProjects.splice(index, 1);
prefs.setValue("recentProjects", recentProjects);
PreferencesManager.setViewState("recentProjects", recentProjects);
checkHovers(e.pageX, e.pageY);

if (recentProjects.length === 1) {
Expand Down
11 changes: 11 additions & 0 deletions src/preferences/PreferencesManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,15 @@ define(function (require, exports, module) {
preferencesManager.save();
}

function getViewState(id) {
return stateManager.get(id);
}

function setViewState(id, value) {
stateManager.set(id, value);
stateManager.save();
}

// Private API for unit testing and use elsewhere in Brackets core
exports._manager = preferencesManager;
exports._setCurrentEditingFile = preferencesManager.setPathScopeContext.bind(preferencesManager);
Expand All @@ -337,6 +346,8 @@ define(function (require, exports, module) {
exports.getPreference = preferencesManager.getPreference.bind(preferencesManager);
exports.getExtensionPrefs = getExtensionPrefs;
exports.setValueAndSave = setValueAndSave;
exports.getViewState = getViewState;
exports.setViewState = setViewState;
exports.addScope = preferencesManager.addScope.bind(preferencesManager);
exports.stateManager = stateManager;
exports.FileStorage = PreferencesBase.FileStorage;
Expand Down
31 changes: 11 additions & 20 deletions src/project/ProjectManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,6 @@ define(function (require, exports, module) {
*/
var _projectBaseUrl = "";

/**
* @private
* @type {PreferenceStorage}
*/
var _prefs = null;

/**
* @private
* Used to initialize jstree state
Expand Down Expand Up @@ -341,7 +335,7 @@ define(function (require, exports, module) {
_projectBaseUrl += "/";
}

_prefs.setValue(_getBaseUrlKey(), _projectBaseUrl);
PreferencesManager.setViewState(_getBaseUrlKey(), _projectBaseUrl);
}

/**
Expand Down Expand Up @@ -390,7 +384,7 @@ define(function (require, exports, module) {
function _savePreferences() {

// save the current project
_prefs.setValue("projectPath", _projectRoot.fullPath);
PreferencesManager.setViewState("projectPath", _projectRoot.fullPath);

// save jstree state
var openNodes = [],
Expand Down Expand Up @@ -425,7 +419,7 @@ define(function (require, exports, module) {
});

// Store the open nodes by their full path and persist to storage
_prefs.setValue(_getTreeStateKey(_projectRoot.fullPath), openNodes);
PreferencesManager.setViewState(_getTreeStateKey(_projectRoot.fullPath), openNodes);
}

/**
Expand Down Expand Up @@ -928,7 +922,7 @@ define(function (require, exports, module) {
return true;
}
var pathNoSlash = FileUtils.stripTrailingSlash(path); // "welcomeProjects" pref has standardized on no trailing "/"
var welcomeProjects = _prefs.getValue("welcomeProjects") || [];
var welcomeProjects = PreferencesManager.getViewState("welcomeProjects") || [];
return welcomeProjects.indexOf(pathNoSlash) !== -1;
}

Expand All @@ -938,10 +932,10 @@ define(function (require, exports, module) {
function addWelcomeProjectPath(path) {
var pathNoSlash = FileUtils.stripTrailingSlash(path); // "welcomeProjects" pref has standardized on no trailing "/"

var welcomeProjects = _prefs.getValue("welcomeProjects") || [];
var welcomeProjects = PreferencesManager.getViewState("welcomeProjects") || [];
if (welcomeProjects.indexOf(pathNoSlash) === -1) {
welcomeProjects.push(pathNoSlash);
_prefs.setValue("welcomeProjects", welcomeProjects);
PreferencesManager.setViewState("welcomeProjects", welcomeProjects);
}
}

Expand All @@ -961,7 +955,7 @@ define(function (require, exports, module) {
* first launch.
*/
function getInitialProjectPath() {
return updateWelcomeProjectPath(_prefs.getValue("projectPath"));
return updateWelcomeProjectPath(PreferencesManager.getViewState("projectPath"));
}

/**
Expand Down Expand Up @@ -1075,7 +1069,7 @@ define(function (require, exports, module) {
};

// restore project tree state from last time this project was open
_projectInitialLoad.previous = _prefs.getValue(_getTreeStateKey(rootPath)) || [];
_projectInitialLoad.previous = PreferencesManager.getViewState(_getTreeStateKey(rootPath)) || [];

// Populate file tree as long as we aren't running in the browser
if (!brackets.inBrowser) {
Expand All @@ -1095,7 +1089,7 @@ define(function (require, exports, module) {
var perfTimerName = PerfUtils.markStart("Load Project: " + rootPath);

_projectRoot = rootEntry;
_projectBaseUrl = _prefs.getValue(_getBaseUrlKey()) || "";
_projectBaseUrl = PreferencesManager.getViewState(_getBaseUrlKey()) || "";

// If this is the most current welcome project, record it. In future launches, we want
// to substitute the latest welcome project from the current build instead of using an
Expand Down Expand Up @@ -2139,11 +2133,8 @@ define(function (require, exports, module) {
});
});

// Init PreferenceStorage
var defaults = {
projectPath: _getWelcomeProjectPath() /* initialize to welcome project */
};
_prefs = PreferencesManager.getPreferenceStorage(module, defaults);
// Init default project path to welcome project
PreferencesManager.stateManager.definePreference("projectPath", "string", _getWelcomeProjectPath());

// Event Handlers
$(FileViewController).on("documentSelectionFocusChange", _documentSelectionFocusChange);
Expand Down
30 changes: 18 additions & 12 deletions src/project/WorkingSetSort.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,6 @@ define(function (require, exports, module) {
automaticSort: false
};

/**
* @private
* @type {PreferenceStorage}
*/
var _prefs = {};

/**
* @private
* @type {Array.<Sort>}
Expand Down Expand Up @@ -116,7 +110,7 @@ define(function (require, exports, module) {
*/
function setAutomatic(enable) {
_automaticSort = enable;
_prefs.setValue("automaticSort", _automaticSort);
PreferencesManager.setValueAndSave("automaticSort", _automaticSort);
CommandManager.get(Commands.SORT_WORKINGSET_AUTO).setChecked(_automaticSort);

if (enable) {
Expand Down Expand Up @@ -164,7 +158,7 @@ define(function (require, exports, module) {

CommandManager.get(Commands.SORT_WORKINGSET_AUTO).setEnabled(!!newSort.getEvents());
_currentSort = newSort;
_prefs.setValue("currentSort", _currentSort.getCommandID());
PreferencesManager.setValueAndSave("currentSort", _currentSort.getCommandID());
}
}

Expand Down Expand Up @@ -321,13 +315,25 @@ define(function (require, exports, module) {
CommandManager.register(Strings.CMD_SORT_WORKINGSET_AUTO, Commands.SORT_WORKINGSET_AUTO, _handleAutomaticSort);


// Initialize PreferenceStorage
_prefs = PreferencesManager.getPreferenceStorage(module, defaultPrefs);
// Initialize default values for sorting preferences
PreferencesManager.definePreference("currentSort", "string", Commands.SORT_WORKINGSET_BY_ADDED);

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

Note that these don't listen for events, so if the user changes them by editing the file (or if they are automatically changed by switching projects), this wouldn't catch that.

Most preferences should listen for changes.

This comment has been minimized.

Sign in to view

Copy link
@RaymondLim

RaymondLim Feb 3, 2014

Author Contributor

You mentioned two scenarios where we would like to listen for events -- one for external edits to the preference file and the other for switching between project scopes. I can agree to listen for events to project level preferences changes, but not sure how to define a project scope and create ".state.json" yet. For the external edits I don't think it make much sense for most of the view states. For example, if the user makes a change to "projectPath" in states.json, I don't think we want to listen to this change and then switch the current project. However, I can see some view states do make sense since we do have a menu item for Live Preview Highlight and it has to be updated as we migrate this view state. (On a related note, I changed some preferences to view states after talking to @peterflynn. Those I changed are the two preferences here, livedev.highlight and two preferences from Find bar.)
PreferencesManager.definePreference("automaticSort", "boolean", false);

/**
* @private
*
* Manage the conversion from old-style localStorage prefs to the new file-based ones.
*/
function _convertPreferences() {
PreferencesManager.convertPreferences(module, {"currentSort": "user", "automaticSort": "user"});
}

_convertPreferences();

// Initialize items dependent on extensions/workingSet
AppInit.appReady(function () {
var curSort = get(_prefs.getValue("currentSort")),
autoSort = _prefs.getValue("automaticSort");
var curSort = get(PreferencesManager.get("currentSort")),
autoSort = PreferencesManager.get("automaticSort");

if (curSort) {
_setCurrentSort(curSort);
Expand Down
26 changes: 17 additions & 9 deletions src/search/FindReplace.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,6 @@ define(function (require, exports, module) {
/** @const Maximum number of matches to collect for Replace All; any additional matches are not listed in the panel & are not replaced */
var REPLACE_ALL_MAX = 300;

var _prefs = PreferencesManager.getPreferenceStorage(module, {
caseSensitive: false,
regexp: false
});

/** @type {!Panel} Panel that shows results of replaceAll action */
var replaceAllPanel = null;

Expand All @@ -89,6 +84,8 @@ define(function (require, exports, module) {
/** @type {!function():void} API from FindInFiles for closing its conflicting search bar, if open */
var closeFindInFilesBar;

PreferencesManager.definePreference("caseSensitive", "boolean", false);
PreferencesManager.definePreference("regexp", "boolean", false);

function SearchState() {
this.searchStartPos = null;
Expand All @@ -110,12 +107,12 @@ define(function (require, exports, module) {
}

function _updateSearchBarFromPrefs() {
$("#find-case-sensitive").toggleClass("active", _prefs.getValue("caseSensitive"));
$("#find-regexp").toggleClass("active", _prefs.getValue("regexp"));
$("#find-case-sensitive").toggleClass("active", PreferencesManager.get("caseSensitive"));
$("#find-regexp").toggleClass("active", PreferencesManager.get("regexp"));
}
function _updatePrefsFromSearchBar() {
_prefs.setValue("caseSensitive", $("#find-case-sensitive").is(".active"));
_prefs.setValue("regexp", $("#find-regexp").is(".active"));
PreferencesManager.setValueAndSave("caseSensitive", $("#find-case-sensitive").is(".active"));
PreferencesManager.setValueAndSave("regexp", $("#find-regexp").is(".active"));
}

function parseQuery(query) {
Expand Down Expand Up @@ -668,6 +665,17 @@ define(function (require, exports, module) {
}
}

/**
* @private
*
* Manage the conversion from old-style localStorage prefs to the new file-based ones.
*/
function _convertPreferences() {
PreferencesManager.convertPreferences(module, {"caseSensitive": "user", "regexp": "user"});

This comment has been minimized.

Sign in to view

Copy link
@dangoor

dangoor Jan 30, 2014

Contributor

It doesn't seem like you need a function wrapper if the conversion is just a single call to convertPreferences and doesn't define any variables

This comment has been minimized.

Sign in to view

Copy link
@RaymondLim

RaymondLim Feb 3, 2014

Author Contributor

Fixed.
}

_convertPreferences();

// Initialize items dependent on HTML DOM
AppInit.htmlReady(function () {
var panelHtml = Mustache.render(searchReplacePanelTemplate, Strings);
Expand Down
Loading

0 comments on commit 4eee5ea

Please sign in to comment.