831916 - Story - Hide and delete tiles in Bookmarks and History tile groups

Status: VERIFIED FIXED

(Tracking Graveyard :: Metro Operations, defect, P2)

Description

[Asa Dotzler [:asa]]

Attachment: UC-52 Hide and delete tiles in Bookmarks and History tile groups

Comment 1

[Matt Brubeck (:mbrubeck)]

Asa, should the hide/delete actions display a "restore" (undo) bar, like the corresponding unpin action for Top Sites (bug 831918)?

Comment 2

[Asa Dotzler [:asa]]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Asa, should the hide/delete actions display a "restore" (undo) bar, like the
> corresponding unpin action for Top Sites (bug 831918)?

Good call. I think we want that for consistency. Yuan, what do you think?

Comment 3

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Agree. I think it makes sense to support undo/restore for bookmarks and history tiles too.

Comment 4

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: WIP alpha

WIP

Added pin/unpin and delete actions to context menu for bookmarks.

For recent history I added only a delete action, working on pin/unpin.

Also fixed context menu action on PanelUI since we're keeping them.

Comment 5

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1 (no tests)

- Added pin/unpin to history items.
- Moved some dependencies to properties to facilitate testing
- Added a refresh option to update items in grid

Will work on adding tests.

I've removed the item count limit on the history panel for testing and ended up liking it and kept it there. Our current limit for the panel was the same as for the start which is 16. I think it's useful to see your history from longer back.

If the history just keeps growing forever then it will eventually be unmanageable and we should just have a larger limit, if there is an expiration  then we might be OK keeping it limitless. I'll investigate further, post my finding here then flag Asa.

Ally, the refresh parameter will be useful in Bug 789363, you can call populateGrid(true) to rearrange startUI and panelUI after adding/deleting items.

Comment 6

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v2

Now with 1000% more tests!

There is a good amount of duplicate code, but there were many little differences that would make writing a common interface very laborious and risky. If you think it's worth it, I can create a refactoring bug.

There are a couple of todo's on the test that depend on Bug 789363 landing.

I'm using nsIAnnotationService to mark tiles as pinned/unpinned. I'm not sure if this will sync or if there is a better solution.

Comment 7

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #5)
> I've removed the item count limit on the history panel for testing and ended
> up liking it and kept it there. Our current limit for the panel was the same
> as for the start which is 16. I think it's useful to see your history from
> longer back.
> 
> If the history just keeps growing forever then it will eventually be
> unmanageable and we should just have a larger limit, if there is an
> expiration  then we might be OK keeping it limitless. I'll investigate
> further, post my finding here then flag Asa.

Asa, there doesn't seem to be an expiration on history. Do you want to have a limit on the amount of tiles or maybe show only a month/week's worth of history?

Comment 8

[Asa Dotzler [:asa]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #7)
> (In reply to Rodrigo Silveira [:rsilveira] from comment #5)
> > I've removed the item count limit on the history panel for testing and ended
> > up liking it and kept it there. Our current limit for the panel was the same
> > as for the start which is 16. I think it's useful to see your history from
> > longer back.
> > 
> > If the history just keeps growing forever then it will eventually be
> > unmanageable and we should just have a larger limit, if there is an
> > expiration  then we might be OK keeping it limitless. I'll investigate
> > further, post my finding here then flag Asa.
> 
> Asa, there doesn't seem to be an expiration on history. Do you want to have
> a limit on the amount of tiles or maybe show only a month/week's worth of
> history?

I think for  now we should trim it to some number that show up in Firefox Start. Later we'll be doing  smart semantic  zoom cool things but for now, it  shouldn't just keep taking more and more Firefox  Start space. 

Yuan, what's a good cap for the number of history tiles to show on Firefox Start?

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

Sorry, I was referring to the panelUI, not the startUI. On the startUI the limit is 16 which is fine. PanelUI currently has the same limit and I think it's useful to have a larger number of tiles there.

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Rodrigo Silveira [:rsilveira] from comment #9)
> Sorry, I was referring to the panelUI, not the startUI. On the startUI the
> limit is 16 which is fine. PanelUI currently has the same limit and I think
> it's useful to have a larger number of tiles there.

For v1 we will only be exposing those "PanelUI" views as part of the snapped start screen, so I think we should still limit them like we do in the full start screen.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 742493 [details] [diff] [review]
Patch v2

Review of attachment 742493 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bindings/grid.xml
@@ -668,4 @@
>        <property name="contextActions">
>          <getter>
>            <![CDATA[
> -            if(!this._contextActions) {

I'd prefer to keep this caching around unless it's broken or causing problems.  I don't know if it's strictly needed, but at least it should prevent us from allocating objects and generating extra garbage every time this property is accessed.

It does mean we need to call item.refresh() every time we change the context actions, but since we generally need to call it for other reasons anyway (sigh) this is not a great loss.

::: browser/metro/base/content/bookmarks.js
@@ +119,5 @@
>    set root(aRoot) {
>      this._root = aRoot;
> +  },
> +
> +  get annotationService() {

Note: PlacesUtils.annotations is an existing lazy getter for this service.

@@ +153,5 @@
> +
> +  _setUnpinned: function bv__setPinned(aItemId) {
> +    this.annotationService.setItemAnnotation(aItemId, kUnpinnedAnnotationName,
> +      true, 0, this.annotationService.EXPIRE_NEVER);
> +  },

I've been warned that the annotation service may have performance issues; for example it uses synchronous I/O (bug 699844).  Let's consider using JSON stored in a pref like the TopSites pin/hide lists in NewTabUtils.jsm.

On the other hand, annotation-service is used in a variety of places in desktop and Metro Firefox, so if it is a win in other ways, maybe we should just be prepared to deal with perf problems we find, and switch to the async API when it is available.

@@ +240,1 @@
>      this._updateFavicon(aBookmarkId, item, uri);

We should add "if (item.refresh) item.refresh();" here in case the binding is attached before these attributes are set.

@@ +364,5 @@
> +      default:
> +        return;
> +    }
> +
> +    this._sendNeedsRefresh();

It seems a little weird to have the object sending an event to itself.  Since no other code seems to listen to this event, should we just call this.getBookmarks(true) here?

::: browser/metro/base/content/history.js
@@ +77,5 @@
> +        // If we're not refreshing or the item is not in the grid, add it.
> +        this.addItemToSet(uri, title, node.icon, addedCount);
> +      } else if (aRefresh && item) {
> +        // Update context action in case it changed in another view.
> +        this._setContextActions(item);

if (item.refresh) item.refresh();

@@ +107,5 @@
> +
> +  addItemToSet: function addItemToSet(aURI, aTitle, aIcon, aPos) {
> +    let item = this._set.insertItemAt(aPos || 0, aTitle, aURI);
> +    item.setAttribute("iconURI", aIcon);
> +    this._setContextActions(item);

if (item.refresh) item.refresh();

@@ +137,5 @@
> +  },
> +
> +  _getItemFromUri: function bv__getItemFromUri(aUri) {
> +    return this._set.querySelector("richgriditem[value='" + aUri + "']");
> +  },

This was just added to richgrid in bug 865478, so you can use that method now.

@@ +219,5 @@
> +
> +          // Clear context app bar
> +          let event = document.createEvent("Events");
> +          event.initEvent("MozContextActionsChange", true, false);
> +          this._set.dispatchEvent(event);

Is this necessary?  I think the context buttons should update automatically when the selection changes as a result of clearSelection below.

If that's not working, we should fix the appbar to clear automatically when it dismisses.

@@ +298,5 @@
>      this._grid.arrangeItems();
>    },
>  
>    init: function init() {
> +    this._view = new HistoryView(this._grid, null, false);

As mentioned above, I think we should use the same limit here as on the regular start page grid.

Comment 12

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> I've been warned that the annotation service may have performance issues;
> for example it uses synchronous I/O (bug 699844).  Let's consider using JSON
> stored in a pref like the TopSites pin/hide lists in NewTabUtils.jsm.

Here's a timely discussion on dev-platform:
https://groups.google.com/d/topic/mozilla.dev.platform/vYbQqkqGzlo/discussion

Comment 13

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> (In reply to Rodrigo Silveira [:rsilveira] from comment #9)
> > Sorry, I was referring to the panelUI, not the startUI. On the startUI the
> > limit is 16 which is fine. PanelUI currently has the same limit and I think
> > it's useful to have a larger number of tiles there.
> 
> For v1 we will only be exposing those "PanelUI" views as part of the snapped
> start screen, so I think we should still limit them like we do in the full
> start screen.

Now I'm confused :). I though we were keeping them on start screen also after the discussion, and I thought bug 864408 would be won't fixed.

With this patch the panel UI is very usable, I don't see a need to remove them. They're the only way to repin a bookmark/history item back to the start screen.

I'll keep the same limit as before then.

Comment 14

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> ::: browser/metro/base/content/bindings/grid.xml
> @@ -668,4 @@
> >        <property name="contextActions">
> >          <getter>
> >            <![CDATA[
> > -            if(!this._contextActions) {
> 
> I'd prefer to keep this caching around unless it's broken or causing
> problems.  I don't know if it's strictly needed, but at least it should
> prevent us from allocating objects and generating extra garbage every time
> this property is accessed.
> 
> It does mean we need to call item.refresh() every time we change the context
> actions, but since we generally need to call it for other reasons anyway
> (sigh) this is not a great loss.
> 

Fair enough, I didn't notice there was a refresh! Done.

> @@ +153,5 @@
> > +
> > +  _setUnpinned: function bv__setPinned(aItemId) {
> > +    this.annotationService.setItemAnnotation(aItemId, kUnpinnedAnnotationName,
> > +      true, 0, this.annotationService.EXPIRE_NEVER);
> > +  },
> 
> I've been warned that the annotation service may have performance issues;
> for example it uses synchronous I/O (bug 699844).  Let's consider using JSON
> stored in a pref like the TopSites pin/hide lists in NewTabUtils.jsm.
> 
> On the other hand, annotation-service is used in a variety of places in
> desktop and Metro Firefox, so if it is a win in other ways, maybe we should
> just be prepared to deal with perf problems we find, and switch to the async
> API when it is available.
> 

OK, I've switched to pref after IRC discussion.

> @@ +240,1 @@
> >      this._updateFavicon(aBookmarkId, item, uri);
> 
> We should add "if (item.refresh) item.refresh();" here in case the binding
> is attached before these attributes are set.
> 

Done.

> @@ +364,5 @@
> > +      default:
> > +        return;
> > +    }
> > +
> > +    this._sendNeedsRefresh();
> 
> It seems a little weird to have the object sending an event to itself. 
> Since no other code seems to listen to this event, should we just call
> this.getBookmarks(true) here?
> 

This is to make all instances of the view call getBookmarks(). So that StartUI and PanelUI are in sync. I'll add a comment to clarify.

> ::: browser/metro/base/content/history.js
> @@ +77,5 @@
> > +        // If we're not refreshing or the item is not in the grid, add it.
> > +        this.addItemToSet(uri, title, node.icon, addedCount);
> > +      } else if (aRefresh && item) {
> > +        // Update context action in case it changed in another view.
> > +        this._setContextActions(item);
> 
> if (item.refresh) item.refresh();
> 
> @@ +107,5 @@
> > +
> > +  addItemToSet: function addItemToSet(aURI, aTitle, aIcon, aPos) {
> > +    let item = this._set.insertItemAt(aPos || 0, aTitle, aURI);
> > +    item.setAttribute("iconURI", aIcon);
> > +    this._setContextActions(item);
> 
> if (item.refresh) item.refresh();
> 

Done.

> @@ +137,5 @@
> > +  },
> > +
> > +  _getItemFromUri: function bv__getItemFromUri(aUri) {
> > +    return this._set.querySelector("richgriditem[value='" + aUri + "']");
> > +  },
> 
> This was just added to richgrid in bug 865478, so you can use that method
> now.
> 

Cool, will sync and use the new method.

> @@ +219,5 @@
> > +
> > +          // Clear context app bar
> > +          let event = document.createEvent("Events");
> > +          event.initEvent("MozContextActionsChange", true, false);
> > +          this._set.dispatchEvent(event);
> 
> Is this necessary?  I think the context buttons should update automatically
> when the selection changes as a result of clearSelection below.
> 
> If that's not working, we should fix the appbar to clear automatically when
> it dismisses.
> 

This is needed to remove the restore button from the appbar after we dismiss. The button is added through a "MozContextActionsChange" event and it's not cleared automatically.

Thanks for the thorough review! I'll sync with Ally's and Sam's changes, fix the tests and attach an updated patch.

Comment 15

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v3

- Removed grid changes and added item.refresh when setting context actions.
- Merged with Ally's and Sam's changes.
- Changed pinned storage to prefs instead of annotations.
- Fixed tests.

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 742733 [details] [diff] [review]
Patch v3

Review of attachment 742733 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck with some trivial changes.

::: browser/metro/base/content/helperui/ItemPinHelper.js
@@ +10,5 @@
> +}
> +
> +// Cache preferences on a static variable shared
> +// by all instances registered to the same pref key.
> +ItemPinHelper._prefValue = [];

Could you change this from [] to {} please?  It might not make any real difference, but it makes the intent clearer to me.

@@ +21,5 @@
> +    try {
> +      // getComplexValue throws if pref never set. Really.
> +      let prefValue = Services.prefs.getComplexValue(this._prefKey, Ci.nsISupportsString);
> +      ItemPinHelper._prefValue[this._prefKey] = JSON.parse(prefValue.data);
> +    }catch(e) {

whitespace nit: space before "catch"

::: browser/metro/base/tests/mochitest/head.js
@@ +155,5 @@
>  
>  function hideContextUI()
>  {
>    purgeEventQueue();
> +  if (ContextUI.isVisible && ContextUI.isExpanded) {

Is this change correct?  What if the ContextUI is visible but *not* expanded - shouldn't we still do something here?

Comment 17

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Matt Brubeck (:mbrubeck) from comment #16)
> ::: browser/metro/base/tests/mochitest/head.js
> @@ +155,5 @@
> >  
> >  function hideContextUI()
> >  {
> >    purgeEventQueue();
> > +  if (ContextUI.isVisible && ContextUI.isExpanded) {
> 
> Is this change correct?  What if the ContextUI is visible but *not* expanded
> - shouldn't we still do something here?

It was not correct! The issue I was fixing was that if the tray is visible but not expanded on start UI, dismiss is no-op:
https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#1219
This wasn't supposed to be there, I missed a qrefresh :). I've changed it to check ContextUI.dismiss() return value before waiting.

Thanks for the weekend review!

Comment 18

[Rodrigo Silveira [:rsilveira] [:rodms]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3d0552005c

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d3d0552005c

Comment 20

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20130429 Firefox/23.0

I tried to verify the story but there is no Hide or Delete option for a selected Bookmarks or History tile, or group of tiles, in context app bar as suggested in attached Use Case.


Any thoughts? Should I fill a new bug for this behavior?

Comment 21

[Marco Mucci [:MarcoM]]

Hi Virgil.  Yes, please file a defect for this.

Comment 22

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Virgil Dicu [:virgil] [QA] from comment #20)
> Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20130429 Firefox/23.0
> 
> I tried to verify the story but there is no Hide or Delete option for a
> selected Bookmarks or History tile, or group of tiles, in context app bar as
> suggested in attached Use Case.
> 
> 
> Any thoughts? Should I fill a new bug for this behavior?

I'm seeing it on my latest build. Maybe the change is not be present on your build? It landed on m-c only yesterday 04-29.

Comment 23

[Virgil Dicu [:virgil] [QA]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #22)
> I'm seeing it on my latest build. Maybe the change is not be present on your
> build? It landed on m-c only yesterday 04-29.

Yeah, sorry, seems so. tried with the 29th build, initially thought it landed in the 16th (changeset date).

Verified with Mozilla/5.0 (Windows NT 6.2; rv:23.0) Gecko/20130501 Firefox/23.0

-app bar displayed at right click on any History or bookmarks tile
-can hide/unhide and delete tiles individually - sites will remain displayed in the Bookmark/ History menu if hidden
-sites are removed from history if deleted - can no longer find results for deleted sites using the address bar.
-can pin/unpin/delete multiple sites at a time
-refresh button is displayed if sites with different states are selected
-can access any site from within Bookmarks/History/Awesome screen.

Comment 24

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Verified in it7.

Filed bug 876030.
Filed bug 876033.

Comment 25

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130707 Firefox/25.0
Build ID: 20130707031138

Tested on Windows 8.1 preview for iteration-9 using latest nightly build from ftp://ftp.mozilla.org/pub/firefox/nightly/2013/07/2013-07-07-03-11-38-mozilla-central/

I used same steps given in user story, but when I delete any tile, the tile is removed from screen but not from browser's history.