489351 - Date containers under History in Library cannot be deleted/cut

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre ID:20090420031111

With the implementation of the date subfolders under the history in the Library (bug 422163) we split up history entries by date now. When you open the context menu of such an date subfolder you will not be able to cut or delete this folder including all the traces beneath. There is no error shown in the Error Console.

Comment 1

[Marco Bonardo [:mak]]

not a regression, because we never had date containers in Library.
Notice this is only for the left pane.
I just did found the problem in controller.js, taking.

Comment 2

[Marco Bonardo [:mak]]

moving to places since it's our controller.

Comment 3

[Dietrich Ayala (:dietrich)]

This is not a regression, so I don't think we'd hold the release for this.

That said, it be a good polish point to have this be consistent with the pre-existing sidebar containers so marking wanted+.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

disable cut when we have an history only node, there's no reason to do that.
and fix delete.

Comment 5

[Marco Bonardo [:mak]]

Drew, about disabling cut, there are currently these reasons to do so:
- drag&drop from history always forces a copy op, so this is consistent with that, and i think the two should stay in sync.
- the action a user would do with a cut from history would be "paste this pages somewhere and then also remove them from history", that is a complex action that would probably confuse users rather then helping them
- History removals are not undoable, and due to bug 484578 there's an high possibility users will have a dataloss thinking cut won't remove their history
- due to bug 486740 and bug 484578 all pasted pages will also lose their titles

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 373958 [details] [diff] [review]
patch v1.0

let's try asking some feedback to Alex.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 373958 [details] [diff] [review]
patch v1.0

OK, I think you convinced me about cut.  You're right that file managers are a better analogy to Places than text in a text editor.  The file managers on Windows, OS X, and Gnome all either don't delete a file until paste or disallow cutting in the first place.  Not sure I agree with that, but it would be wiser to follow their lead, and it's not a big deal.

Patch doesn't apply cleanly to trunk, so I didn't test it yet, but just looking over the code...

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js

>@@ -922,13 +929,20 @@ PlacesController.prototype = {
>           transactions.push(PlacesUIUtils.ptm.untagURI(URIs[j], [tag]));
>         continue;
>       }
>-      else if (PlacesUtils.nodeIsQuery(node.parent) &&
>+      else if (PlacesUtils.nodeIsURI(node) &&
>+               PlacesUtils.nodeIsQuery(node.parent) &&
>                asQuery(node.parent).queryOptions.queryType ==
>-                Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY &&
>-               node.uri) {
>-        // remove page from history, history deletes are not undoable
>+                 Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) {
>         var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);
>         bhist.removePage(PlacesUtils._uri(node.uri));
>+        // History deletes are not undoable, so we don't have a transaction.
>+        continue;
>+      }
>+      else if (PlacesUtils.nodeIsQuery(node) &&
>+               asQuery(node).queryOptions.queryType ==
>+                 Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) {
>+        this._removeHistoryContainer(node);
>+        // History deletes are not undoable, so we don't have a transaction.
>         continue;
>       }

A comment above each of these conditionals (and with this patch there are now five) explaining what kind of nodes they catch would be helpful.

>@@ -969,24 +983,17 @@ PlacesController.prototype = {
> 
>     for (var i = 0; i < nodes.length; ++i) {
>       var node = nodes[i];
>-      if (PlacesUtils.nodeIsHost(node))
>-        bhist.removePagesFromHost(node.title, true);
>-      else if (PlacesUtils.nodeIsURI(node)) {
>+        if (PlacesUtils.nodeIsURI(node)) {

Indentation.

>+      // We want to exclude beginTime from the removal
>+      bhist.removePagesByTimeframe(beginTime+1, endTime);

I realize this was already in the code, but why is that?  Does this not exclude the oldest item in the container?

>diff --git a/browser/components/places/tests/browser/browser_library_left_pane_commands.js b/browser/components/places/tests/browser/browser_library_left_pane_commands.js

Do you plan on adding more tests to this file later?  Its name and the comment at the top imply more than it actually covers.

>+    // Check that we have a child container. It is "Today" container.
>+    is(historyNode.childCount, 1, "History node has one child");
>+    var todayNode = historyNode.getChild(0);
>+    is(todayNode.title, "Today", "History child is Today container");

I'd like to see this not rely on the "Today" string.  Just because we may have many tests relying on en-US doesn't mean we should keep piling them up.  At least you could get the "finduri-AgeInDays-is-0" property instead, right?

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 373958 [details] [diff] [review]
patch v1.0

behavior sounds fine

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #7)
> Patch doesn't apply cleanly to trunk, so I didn't test it yet, but just looking
> over the code...

could be some recent checkin has changed, or i coded on a personal queue :\ will post an updated one soon.

> >diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
> >+      // We want to exclude beginTime from the removal
> >+      bhist.removePagesByTimeframe(beginTime+1, endTime);
> 
> I realize this was already in the code, but why is that?  Does this not exclude
> the oldest item in the container?

removePagesByTimeframe removes considering extremes included, but date containers are begin < content <= end so we would end up removing more than needed.
I'll add a comment, i agree is not so much clear.

> >diff --git a/browser/components/places/tests/browser/browser_library_left_pane_commands.js b/browser/components/places/tests/browser/browser_library_left_pane_commands.js
> 
> Do you plan on adding more tests to this file later?  Its name and the comment
> at the top imply more than it actually covers.

Well yes i've structired it to receive further tests in future, i think when possible we should group tests for bugs covering the same UI part. I shoul file a followup to create tests there.

> >+    // Check that we have a child container. It is "Today" container.
> >+    is(historyNode.childCount, 1, "History node has one child");
> >+    var todayNode = historyNode.getChild(0);
> >+    is(todayNode.title, "Today", "History child is Today container");
> 
> I'd like to see this not rely on the "Today" string.  Just because we may have
> many tests relying on en-US doesn't mean we should keep piling them up.  At
> least you could get the "finduri-AgeInDays-is-0" property instead, right?

yes, i'll do!

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 374180 [details] [diff] [review]
patch v1.1

>+        // This is am uri nodes inside a tag container.  It needs a special

Nit: "This is a URI node"

>+        // This is an uri node inside an history query.

Ditto.

>+    todayNodeExpectedTitle = PlacesUtils.getString("finduri-AgeInDays-is-0");

var todayNodeExpectedTitle

r=adw

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.2

addressed comments.

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/0498e448db64

Comment 14

[Jim Jeffery not reading bug-mail 1/2/11]

I believe, and its been mentioned that this bug broke the right-click 'context' menu for Bookmarks Menu.. 

click bookmarks menu
look at any bookmark 
right-click -<Note: no context menu. 

Reported here in the forums:
http://forums.mozillazine.org/viewtopic.php?p=6338695#p6338695

Comment 15

[Marco Bonardo [:mak]]

(In reply to comment #14)
> I believe, and its been mentioned that this bug broke the right-click 'context'
> menu for Bookmarks Menu.. 

that's Bug 490158, not caused by this one.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 374247 [details] [diff] [review]
patch v1.2

asking approval for 1.9.1, fixes some missing check in removal code, so that now cut and delete are correctly enabled/disabled on dynamically created queries. Requires approval on 490156 too.

Comment 17

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 374247 [details] [diff] [review]
patch v1.2

> (From update of attachment 375058 [details] [diff] [review])
> fixes a regression from bug 489351, that should be approved with this.

We can't keep tracing through your dependency trees this way; something's going to be missed. Please give us a roll-up patch and ask for approval in a single place if this sort of thing comes up.

Comment 19

[Marco Bonardo [:mak]]

Attachment: patch for 1.9.1, includes patch for regression bug 490156

you're right, this is merged with regression fix.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 377215 [details] [diff] [review]
patch for 1.9.1, includes patch for regression bug 490156 

a191=beltzner

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e1bf18953337

Comment 22

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre ID:20090521030957

Comment 23

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv