Closed
Bug 489351
Opened 15 years ago
Closed 15 years ago
Date containers under History in Library cannot be deleted/cut
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: whimboo, Assigned: mak)
References
Details
(Keywords: verified1.9.1)
Attachments
(2 files, 2 obsolete files)
12.55 KB,
patch
|
beltzner
:
approval1.9.1-
|
Details | Diff | Splinter Review |
15.67 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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?
Updated•15 years ago
|
Summary: Date containers under History in Library cannot be deleted/cutted → Date containers under History in Library cannot be deleted/cut
Assignee | ||
Comment 1•15 years ago
|
||
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
Assignee | ||
Comment 2•15 years ago
|
||
moving to places since it's our controller.
Comment 3•15 years ago
|
||
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-
Assignee | ||
Comment 4•15 years ago
|
||
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 | ||
Updated•15 years ago
|
Assignee: nobody → mak77
Updated•15 years ago
|
Attachment #373958 -
Flags: review?(dietrich) → review?(adw)
Assignee | ||
Comment 5•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #373958 -
Flags: ui-review?(faaborg)
Assignee | ||
Comment 6•15 years ago
|
||
Comment on attachment 373958 [details] [diff] [review] patch v1.0 let's try asking some feedback to Alex.
Comment 7•15 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?
Updated•15 years ago
|
Attachment #373958 -
Flags: ui-review?(faaborg) → ui-review+
Comment 8•15 years ago
|
||
Comment on attachment 373958 [details] [diff] [review] patch v1.0 behavior sounds fine
Assignee | ||
Comment 9•15 years ago
|
||
(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!
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #373958 -
Attachment is obsolete: true
Attachment #374180 -
Flags: review?(adw)
Attachment #373958 -
Flags: review?(adw)
Comment 11•15 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+
Assignee | ||
Comment 12•15 years ago
|
||
addressed comments.
Attachment #374180 -
Attachment is obsolete: true
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0498e448db64
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Comment 14•15 years ago
|
||
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
Assignee | ||
Comment 15•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Attachment #374247 -
Flags: approval1.9.1?
Assignee | ||
Comment 16•15 years ago
|
||
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•15 years ago
|
||
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 18•15 years ago
|
||
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-
Assignee | ||
Comment 19•15 years ago
|
||
you're right, this is merged with regression fix.
Attachment #377215 -
Flags: approval1.9.1?
Comment 20•15 years ago
|
||
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+
Assignee | ||
Comment 21•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e1bf18953337
Keywords: fixed1.9.1
Comment 22•15 years ago
|
||
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
Comment 23•15 years ago
|
||
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.
Description
•