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

VERIFIED FIXED

Status

P2
normal
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: asa, Assigned: rsilveira)

Tracking

Details

(Whiteboard: feature=story c=firefox_start u=metro_firefox_user p=8)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 703485 [details]
UC-52 	 Hide and delete tiles in Bookmarks and History tile groups
(Reporter)

Updated

6 years ago
Assignee: nobody → asa
Priority: -- → P1
Whiteboard: c=Awesome_screen u= p= → c=Awesome_screen u=metro_firefox_user p=
(Reporter)

Updated

6 years ago
Assignee: asa → nobody
(Reporter)

Updated

6 years ago
Priority: P1 → P2
(Reporter)

Updated

6 years ago
Summary: Hide and delete tiles in Bookmarks and History tile groups → Story - Hide and delete tiles in Bookmarks and History tile groups
Whiteboard: c=Awesome_screen u=metro_firefox_user p= → c=Awesome_screen u=metro_firefox_user feature=story

Updated

6 years ago
OS: Windows 8 → Windows 8 Metro
Whiteboard: c=Awesome_screen u=metro_firefox_user feature=story → feature=story c=Awesome_screen u=metro_firefox_user p=0

Updated

6 years ago
Blocks: 838081
Asa, should the hide/delete actions display a "restore" (undo) bar, like the corresponding unpin action for Top Sites (bug 831918)?
Flags: needinfo?(asa)
Hardware: x86_64 → All
Whiteboard: feature=story c=Awesome_screen u=metro_firefox_user p=0 → feature=story c=Awesome_screen u=metro_firefox_user p=5
(Reporter)

Comment 2

6 years ago
(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?
Flags: needinfo?(asa) → needinfo?(ywang)
Agree. I think it makes sense to support undo/restore for bookmarks and history tiles too.
Flags: needinfo?(ywang)
Whiteboard: feature=story c=Awesome_screen u=metro_firefox_user p=5 → feature=story c=Awesome_screen u=metro_firefox_user p=8

Updated

6 years ago
Whiteboard: feature=story c=Awesome_screen u=metro_firefox_user p=8 → feature=story c=firefox_start u=metro_firefox_user p=8

Updated

6 years ago
Component: General → Metro Operations
Product: Firefox for Metro → Tracking
Version: unspecified → ---

Updated

6 years ago
Assignee: nobody → rsilveira
Blocks: 855905
No longer blocks: 838081
Status: NEW → ASSIGNED
QA Contact: jbecerra
Created attachment 741153 [details] [diff] [review]
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.
Created attachment 741609 [details] [diff] [review]
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.
Attachment #741153 - Attachment is obsolete: true
Attachment #741609 - Flags: feedback?(mbrubeck)
Attachment #741609 - Flags: feedback?(ally)
Created attachment 742493 [details] [diff] [review]
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.
Attachment #741609 - Attachment is obsolete: true
Attachment #741609 - Flags: feedback?(mbrubeck)
Attachment #741609 - Flags: feedback?(ally)
Attachment #742493 - Flags: review?(mbrubeck)
(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?
Flags: needinfo?(asa)
(Reporter)

Comment 8

6 years ago
(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?
Flags: needinfo?(asa) → needinfo?(ywang)
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.
(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 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.
Attachment #742493 - Flags: review?(mbrubeck) → review-
(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
(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.
(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.
Created attachment 742733 [details] [diff] [review]
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.
Attachment #742493 - Attachment is obsolete: true
Attachment #742733 - Flags: review?(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?
Attachment #742733 - Flags: review?(mbrubeck) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/9d3d0552005c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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?
Flags: needinfo?(asa)
Hi Virgil.  Yes, please file a defect for this.
Flags: needinfo?(asa)
Depends on: 867179
(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.
(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.
Status: RESOLVED → VERIFIED
No longer depends on: 867179
Verified in it7.

Filed bug 876030.
Filed bug 876033.
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.

Updated

5 years ago
Depends on: 891037

Updated

5 years ago
Depends on: 898562

Updated

5 years ago
No longer depends on: 898562
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.