Date containers under History in Library cannot be deleted/cut

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: whimboo, Assigned: mak)

Tracking

({verified1.9.1})

3.5 Branch
Firefox 3.6a1
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 -
wanted-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Flags: in-testsuite?
Flags: blocking-firefox3.5?
Summary: Date containers under History in Library cannot be deleted/cutted → Date containers under History in Library cannot be deleted/cut
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
moving to places since it's our controller.
Assignee: mak77 → nobody
Component: Menus → Places
Keywords: regression
QA Contact: menus → places
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+.
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
Created attachment 373958 [details] [diff] [review]
patch v1.0

disable cut when we have an history only node, there's no reason to do that.
and fix delete.
Attachment #373958 - Flags: review?(dietrich)
Assignee: nobody → mak77
Attachment #373958 - Flags: review?(dietrich) → review?(adw)
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
Attachment #373958 - Flags: ui-review?(faaborg)
Comment on attachment 373958 [details] [diff] [review]
patch v1.0

let's try asking some feedback to Alex.
Keywords: uiwanted

Comment 7

9 years ago
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?
Attachment #373958 - Flags: ui-review?(faaborg) → ui-review+
Comment on attachment 373958 [details] [diff] [review]
patch v1.0

behavior sounds fine
(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!
Keywords: uiwanted
Created attachment 374180 [details] [diff] [review]
patch v1.1
Attachment #373958 - Attachment is obsolete: true
Attachment #374180 - Flags: review?(adw)
Attachment #373958 - Flags: review?(adw)

Comment 11

9 years ago
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
Attachment #374180 - Flags: review?(adw) → review+
Created attachment 374247 [details] [diff] [review]
patch v1.2

addressed comments.
Attachment #374180 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/0498e448db64
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
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
(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.
Depends on: 490156
Attachment #374247 - Flags: approval1.9.1?
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.
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre
Status: RESOLVED → VERIFIED
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.
Attachment #374247 - Flags: approval1.9.1? → approval1.9.1-
Created attachment 377215 [details] [diff] [review]
patch for 1.9.1, includes patch for regression bug 490156 

you're right, this is merged with regression fix.
Attachment #377215 - Flags: approval1.9.1?
Comment on attachment 377215 [details] [diff] [review]
patch for 1.9.1, includes patch for regression bug 490156 

a191=beltzner
Attachment #377215 - Flags: approval1.9.1? → approval1.9.1+
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
Keywords: fixed1.9.1 → verified1.9.1
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.