Commit 3983dda0 authored by Luciano Pacheco's avatar Luciano Pacheco Committed by Commit Bot

Revert "[Reland] Change ActionsController to only update DOM when displaying a menu"

This reverts commit ffcd9342.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> [Reland] Change ActionsController to only update DOM when displaying a menu
> 
> Previously ActionsController would calculate the additional actions for
> Drive (share, create-shortcut, etc) and FSP [1] like change directory,
> selection changed, and metadata updated. When these events arrive in
> FilesApp, they update the <command> and <cr-menu> DOM state.
> 
> However, that can cause flickering in visible menus and also flakes in
> browser tests based on menus, since these events can happen while the
> menu is displayed for an entry or set of entries (selection) or affect
> entries other than those associated with the menu.
> 
> This CL changes ActionsController to store state in an internal map,
> rather than in the DOM, and to only update the menu DOM state when a
> user-action shows the menu.
> 
> Change CommandHandler to set the visibility for Drive actions/commands
> synchronously to avoid flickering the size of the menu - it still can
> flicker the disabled state and pinned check-mark, but these are softer
> to human eyes than the change in size.
> 
> Change ToolbarController to update its refresh button disabled state.
> The button relied on FileManagerCommands to update the |refresh| state
> command, which was not correct because the refresh button always acts
> on the current directory, whereas the command was being evaluated for
> different entries.
> 
> Change Action models and derived classes to have a |getEntries| method
> so that ActionsController can get the entries that it is acting upon
> to invalidate the state prior to executation.
> 
> Change ActionsSubMenu.setActionsModel to forward the target element,
> so the <command> can be updated to the correct target element.
> 
> [1]
> https://developer.chrome.com/apps/fileSystemProvider#event-onGetActionsRequested
> 
> Bug: 889153, 875446, 961244
> Change-Id: I059f4516f0b64426a7570a88d8eff7e48b435454
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1605383
> Reviewed-by: Noel Gordon <noel@chromium.org>
> Commit-Queue: Noel Gordon <noel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#658458}

TBR=noel@chromium.org,lucmult@chromium.org

Change-Id: I67d21f2794ed17279b52f4c4fc90f80936abe881
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 889153, 875446, 961244
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1605662Reviewed-by: default avatarLuciano Pacheco <lucmult@chromium.org>
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658468}
parent 64452100
...@@ -179,8 +179,6 @@ js_library("util") { ...@@ -179,8 +179,6 @@ js_library("util") {
"../../../externs/entry_location.js", "../../../externs/entry_location.js",
"../../../externs/platform.js", "../../../externs/platform.js",
"../../../externs/volume_info.js", "../../../externs/volume_info.js",
"../../../externs/volume_info_list.js",
"../../../externs/volume_manager.js",
] ]
} }
......
...@@ -1549,36 +1549,3 @@ util.entryDebugString = (entry) => { ...@@ -1549,36 +1549,3 @@ util.entryDebugString = (entry) => {
} }
return entryDescription; return entryDescription;
}; };
/**
* Returns true if all entries belong to the same volume. If there are no
* entries it also returns false.
*
* @param {!Array<Entry|FilesAppEntry>} entries
* @param {!VolumeManager} volumeManager
* @return boolean
*/
util.isSameVolume = (entries, volumeManager) => {
if (!entries.length) {
return false;
}
const firstEntry = entries[0];
if (!firstEntry) {
return false;
}
const volumeInfo = volumeManager.getVolumeInfo(firstEntry);
for (let i = 1; i < entries.length; i++) {
if (!entries[i]) {
return false;
}
const volumeInfoToCompare = volumeManager.getVolumeInfo(assert(entries[i]));
if (!volumeInfoToCompare ||
volumeInfoToCompare.volumeId !== volumeInfo.volumeId) {
return false;
}
}
return true;
};
...@@ -23,12 +23,6 @@ class Action { ...@@ -23,12 +23,6 @@ class Action {
* @return {?string} * @return {?string}
*/ */
getTitle() {} getTitle() {}
/**
* Entries that this Action will execute upon.
* @return {!Array<!Entry|!FileEntry>}
*/
getEntries() {}
} }
/** /**
...@@ -133,11 +127,6 @@ class DriveShareAction { ...@@ -133,11 +127,6 @@ class DriveShareAction {
getTitle() { getTitle() {
return null; return null;
} }
/** @override */
getEntries() {
return [this.entry_];
}
} }
...@@ -307,11 +296,6 @@ class DriveToggleOfflineAction { ...@@ -307,11 +296,6 @@ class DriveToggleOfflineAction {
getTitle() { getTitle() {
return null; return null;
} }
/** @override */
getEntries() {
return this.entries_;
}
} }
...@@ -383,11 +367,6 @@ class DriveCreateFolderShortcutAction { ...@@ -383,11 +367,6 @@ class DriveCreateFolderShortcutAction {
getTitle() { getTitle() {
return null; return null;
} }
/** @override */
getEntries() {
return [this.entry_];
}
} }
...@@ -454,11 +433,6 @@ class DriveRemoveFolderShortcutAction { ...@@ -454,11 +433,6 @@ class DriveRemoveFolderShortcutAction {
getTitle() { getTitle() {
return null; return null;
} }
/** @override */
getEntries() {
return [this.entry_];
}
} }
...@@ -552,11 +526,6 @@ class DriveManageAction { ...@@ -552,11 +526,6 @@ class DriveManageAction {
getTitle() { getTitle() {
return null; return null;
} }
/** @override */
getEntries() {
return [this.entry_];
}
} }
...@@ -626,11 +595,6 @@ class CustomAction { ...@@ -626,11 +595,6 @@ class CustomAction {
getTitle() { getTitle() {
return this.title_; return this.title_;
} }
/** @override */
getEntries() {
return this.entries_;
}
} }
/** /**
...@@ -728,13 +692,22 @@ class ActionsModel extends cr.EventTarget { ...@@ -728,13 +692,22 @@ class ActionsModel extends cr.EventTarget {
const volumeInfo = this.entries_.length >= 1 && const volumeInfo = this.entries_.length >= 1 &&
this.volumeManager_.getVolumeInfo(this.entries_[0]); this.volumeManager_.getVolumeInfo(this.entries_[0]);
// All entries need to be on the same volume to execute ActionsModel if (!volumeInfo) {
// commands.
if (!volumeInfo ||
!util.isSameVolume(this.entries_, this.volumeManager_)) {
fulfill({}); fulfill({});
return; return;
} }
// All entries need to be on the same volume to execute ActionsModel
// commands.
// TODO(sashab): Move this to util.js.
for (let i = 1; i < this.entries_.length; i++) {
const volumeInfoToCompare =
this.volumeManager_.getVolumeInfo(this.entries_[i]);
if (!volumeInfoToCompare ||
volumeInfoToCompare.volumeId != volumeInfo.volumeId) {
fulfill({});
return;
}
}
const actions = {}; const actions = {};
switch (volumeInfo.volumeType) { switch (volumeInfo.volumeType) {
...@@ -864,14 +837,6 @@ class ActionsModel extends cr.EventTarget { ...@@ -864,14 +837,6 @@ class ActionsModel extends cr.EventTarget {
} }
cr.dispatchSimpleEvent(this, 'invalidated', true); cr.dispatchSimpleEvent(this, 'invalidated', true);
} }
/**
* @return {!Array<!Entry>}
* @public
*/
getEntries() {
return this.entries_;
}
} }
/** /**
......
...@@ -73,6 +73,10 @@ function GearMenuController( ...@@ -73,6 +73,10 @@ function GearMenuController(
GearMenuController.prototype.onShowGearMenu_ = function() { GearMenuController.prototype.onShowGearMenu_ = function() {
this.toggleRipple_.activated = true; this.toggleRipple_.activated = true;
this.refreshRemainingSpace_(false); /* Without loading caption. */ this.refreshRemainingSpace_(false); /* Without loading caption. */
// Update view of drive-related settings.
this.commandHandler_.updateAvailability();
this.updateNewServiceItem(); this.updateNewServiceItem();
}; };
......
...@@ -62,14 +62,6 @@ class ToolbarController { ...@@ -62,14 +62,6 @@ class ToolbarController {
queryRequiredElement('#delete', assert(this.toolbar_.ownerDocument)), queryRequiredElement('#delete', assert(this.toolbar_.ownerDocument)),
cr.ui.Command); cr.ui.Command);
/**
* @private {!cr.ui.Command}
* @const
*/
this.refreshCommand_ = assertInstanceof(
queryRequiredElement('#refresh', assert(this.toolbar_.ownerDocument)),
cr.ui.Command);
/** /**
* @private {!HTMLElement} * @private {!HTMLElement}
* @const * @const
...@@ -113,9 +105,6 @@ class ToolbarController { ...@@ -113,9 +105,6 @@ class ToolbarController {
this.navigationList_.addEventListener( this.navigationList_.addEventListener(
'relayout', this.onNavigationListRelayout_.bind(this)); 'relayout', this.onNavigationListRelayout_.bind(this));
this.directoryModel_.addEventListener(
'directory-change', this.updateCurrentDirectoryButtons_.bind(this));
// Watch visibility of toolbar buttons to update the width of location line. // Watch visibility of toolbar buttons to update the width of location line.
const observer = const observer =
new MutationObserver(this.onToolbarButtonsMutated_.bind(this)); new MutationObserver(this.onToolbarButtonsMutated_.bind(this));
...@@ -128,20 +117,6 @@ class ToolbarController { ...@@ -128,20 +117,6 @@ class ToolbarController {
} }
} }
/**
* Updates buttons that act on current directory.
* @private
*/
updateCurrentDirectoryButtons_() {
const volumeInfo = this.directoryModel_.getCurrentVolumeInfo();
this.refreshCommand_.disabled = !!volumeInfo && volumeInfo.watchable;
console.log(
'****** toolbar controller.disabled: ' + this.refreshCommand_.disabled);
this.refreshCommand_.setHidden(
volumeInfo && volumeInfo.watchable ||
this.directoryModel_.getFileListSelection().getCheckSelectMode());
}
/** /**
* Handles selection's change event to update the UI. * Handles selection's change event to update the UI.
* @private * @private
......
...@@ -38,9 +38,8 @@ class ActionsSubmenu { ...@@ -38,9 +38,8 @@ class ActionsSubmenu {
/** /**
* @param {ActionsModel} actionsModel * @param {ActionsModel} actionsModel
* @param {Element} element The target element.
*/ */
setActionsModel(actionsModel, element) { setActionsModel(actionsModel) {
this.items_.forEach(item => { this.items_.forEach(item => {
item.parentNode.removeChild(item); item.parentNode.removeChild(item);
}); });
...@@ -62,8 +61,7 @@ class ActionsSubmenu { ...@@ -62,8 +61,7 @@ class ActionsSubmenu {
menuItem.classList.toggle('hide-on-toolbar', true); menuItem.classList.toggle('hide-on-toolbar', true);
delete remainingActions[ActionsModel.CommonActionId.SHARE]; delete remainingActions[ActionsModel.CommonActionId.SHARE];
} }
util.queryDecoratedElement('#share', cr.ui.Command) util.queryDecoratedElement('#share', cr.ui.Command).canExecuteChange();
.canExecuteChange(element);
// Then add the Manage in Drive item (if available). // Then add the Manage in Drive item (if available).
const manageInDriveAction = const manageInDriveAction =
...@@ -75,13 +73,13 @@ class ActionsSubmenu { ...@@ -75,13 +73,13 @@ class ActionsSubmenu {
delete remainingActions[ActionsModel.InternalActionId.MANAGE_IN_DRIVE]; delete remainingActions[ActionsModel.InternalActionId.MANAGE_IN_DRIVE];
} }
util.queryDecoratedElement('#manage-in-drive', cr.ui.Command) util.queryDecoratedElement('#manage-in-drive', cr.ui.Command)
.canExecuteChange(element); .canExecuteChange();
// Removing shortcuts is not rendered in the submenu to keep the previous // Removing shortcuts is not rendered in the submenu to keep the previous
// behavior. Shortcuts can be removed in the left nav using the roots menu. // behavior. Shortcuts can be removed in the left nav using the roots menu.
// TODO(mtomasz): Consider rendering the menu item here for consistency. // TODO(mtomasz): Consider rendering the menu item here for consistency.
util.queryDecoratedElement('#remove-folder-shortcut', cr.ui.Command) util.queryDecoratedElement('#remove-folder-shortcut', cr.ui.Command)
.canExecuteChange(element); .canExecuteChange();
// Both save-for-offline and offline-not-necessary are handled by the single // Both save-for-offline and offline-not-necessary are handled by the single
// #toggle-pinned command. // #toggle-pinned command.
...@@ -101,7 +99,7 @@ class ActionsSubmenu { ...@@ -101,7 +99,7 @@ class ActionsSubmenu {
} }
} }
util.queryDecoratedElement('#toggle-pinned', cr.ui.Command) util.queryDecoratedElement('#toggle-pinned', cr.ui.Command)
.canExecuteChange(element); .canExecuteChange();
// Process all the rest as custom actions. // Process all the rest as custom actions.
Object.keys(remainingActions).forEach(key => { Object.keys(remainingActions).forEach(key => {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment