Closed Bug 323812 Opened 19 years ago Closed 18 years ago

Interesting bookmarks copy-paste behavior

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 2 alpha2

People

(Reporter: bugs, Assigned: bugs)

Details

Attachments

(1 file, 3 obsolete files)

Steps to reproduce:

1. select a bookmark in the places view
2. hit Ctrl+C
3. hit Ctrl+V

Expected results:

- bookmark duplicated

Actual results:

- item adjacent to bookmark vanishes, bookmark remains. Reloading the view shows the view as it was before.
Priority: -- → P2
Attached patch Patch (obsolete) — Splinter Review
Attachment #209886 - Flags: review?(annie.sullivan)
Attachment #209886 - Flags: review?(annie.sullivan) → review+
Comment on attachment 209886 [details] [diff] [review]
Patch

Oops, this patch was actually for bug 322290.
Attachment #209886 - Attachment is obsolete: true
I think this has since been fixed. 
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 2 alpha2
No it hasn't, although there's a new behavior:

- the selection disappears, and nothing is added. 

I've had the browser crash on my once when copying/pasting too.
TB15095194Q
Attached patch patch (obsolete) — Splinter Review
Attachment #216367 - Attachment is obsolete: true
This patch:

- ensures a useful set of commands show up when right clicking in the blank spot on views with no selection (items like new folder, paste, etc)
- makes sure select all isn't enabled if there are no children to select
- makes sure the sorting command isn't enabled unless there are at least two children 
- makes sure the paste command isn't enabled if the URLs on the clipboard are all already in the current folder (Ctrl+C,Ctrl+V should be disabled)
- allow new folders to be created in the left list when items in the sys area are selected, but just insert at the right place
- correctly determine mutability of a container - a container can be mutable even if the active view has no selected items. 
- open a new browser window if necessary for single open actions (opportunistic)
- error checking in unwrapNodes to prevent spurious newlines at the end of a data blob from causing crashes/errors 
- initialize the activeView for the places organizer, so that the commands in the UI work before the user explicitly clicks in a view
- fix the tree insertionPoint getter to handle views without a selection
- fix the tree insertionPoint getter to correctly compute the insertion index when the selection is in the system area (tree vs. container index snafu)
- remove redundant/faulty/incorrect isContainerOpen checking in _getInsertionPoint
- null check to avoid crash in bookmarks observer enumeration code
- change access keys for open commands to avoid collisions in context menus for places organizer.
Attachment #216453 - Flags: review?(annie.sullivan)
Comment on attachment 216453 [details] [diff] [review]
patch


>+    var enoughChildrenToSort = false;
>+    if (this.nodeIsFolder(sortFolder)) {
>+      var folder = asFolder(sortFolder);
>+      var containerWasOpen = folder.containerOpen;
>+      var contents = this.getFolderContents(folder.folderId, false, false);
>+      contents.containerOpen = true;
>+      enoughChildrenToSort = contents.childCount > 1;
>+      contents.containerOpen = false;
>+    }

Looks like containerWasOpen isn't used for anything, shouldn't it be like this:
var containerWasOpen = contents.containerOpen;
contents.containerOpen = true;
enoughChildrenToSort = contents.childCount > 1;
contents.containerOpen = containerWasOpen;

>+      var nodes = this.unwrapNodes(data, type.value);
>+      
>+      var ip = this.activeView.insertionPoint;
>+      var contents = this.getFolderContents(ip.folderId);
>+      var cc = contents.childCount;

Is contents.containerOpen guaranteed to be true here?  If it's not true, it will throw an exception.

>-      var browser = this.browserWindow.getBrowser();
>+      var browser = this._getBrowserWindow().getBrowser();

Can't this._getBrowserWindow() return null?

>-          this.browserWindow.openNewTabWith(nodes[i].uri,
>+          this._getBrowserWindow().openNewTabWith(nodes[i].uri,
>               null, null);

Same here.

>+      if (copy) {
>+        NS_ASSERT(!isNaN(data.folderId), "goats");

Can you be a little more specific than "goats"?


>+            node = resultView.nodeForTreeIndex(max.value);
>+            var container = asContainer(this.getResult().root);
>+            var cc = container.childCount;

Again, is it guaranteed that container is always open?
Attachment #216453 - Flags: review?(annie.sullivan) → review-
(In reply to comment #9)
> (From update of attachment 216453 [details] [diff] [review] [edit])
> 
> Looks like containerWasOpen isn't used for anything, shouldn't it be like this:
> var containerWasOpen = contents.containerOpen;
> contents.containerOpen = true;
> enoughChildrenToSort = contents.childCount > 1;
> contents.containerOpen = containerWasOpen;

Urg, you're right... but getFolderContents guarantees the container is open (updating the docs to reflect this), so none of my dancing was necessary. 

> >+      var ip = this.activeView.insertionPoint;
> >+      var contents = this.getFolderContents(ip.folderId);
> >+      var cc = contents.childCount;
> 
> Is contents.containerOpen guaranteed to be true here?  If it's not true, it
> will throw an exception.

Yes, because getFolderContents opens it. GC should release the result it creates. 

> >-      var browser = this.browserWindow.getBrowser();
> >+      var browser = this._getBrowserWindow().getBrowser();
> 
> Can't this._getBrowserWindow() return null?

Yes, and this was always the case. Fixing it would require non-trivial changes to this function and since this was an opportunistic fix I decided to track that separately. Filed 331908 on it ;-) 

> >+      if (copy) {
> >+        NS_ASSERT(!isNaN(data.folderId), "goats");
> 
> Can you be a little more specific than "goats"?

This was for my own personal use. Removed in latest patch.

> >+            node = resultView.nodeForTreeIndex(max.value);
> >+            var container = asContainer(this.getResult().root);
> >+            var cc = container.childCount;
> 
> Again, is it guaranteed that container is always open?

Question: are tree view roots open? 
Attached patch patchSplinter Review
This patch does a few more things too (sorry, I thought you were busy today so I kept hacking!)

In unwrapNodes:
 - checks that the number of remaining parts is at least as many required to construct a meaningful item object, so spurious newlines don't cause dummy items to be added that cause other parts of the code to screw up. Yes, I'll add a comment to this effect. 

In paste:
- there was a bug where it was not possible to paste more than one kind of item at a time, since the xferable had all flavors added to it, so it would just pick the best one. I needed to refactor the code in this function quite a bit. Basically the clipboard needs to be asked once for every major data type so that folders, separators and items are all pulled. 

In PRFT._saveFolderContents:
- no need to explicitly open the container after getFolderContents
- a bug was causing nested folders not to be deleted since the nested folder remove transactions were being created with the parent folder's folder id! Needed to change that to construct txns with the nested folder's folder id.
Attachment #216453 - Attachment is obsolete: true
Attachment #216468 - Flags: review?(annie.sullivan)
Attachment #216468 - Flags: review?(annie.sullivan) → review+
Fixed, branch and trunk. 
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: