Closed
Bug 323812
Opened 19 years ago
Closed 18 years ago
Interesting bookmarks copy-paste behavior
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 2 alpha2
People
(Reporter: bugs, Assigned: bugs)
Details
Attachments
(1 file, 3 obsolete files)
37.38 KB,
patch
|
annie.sullivan
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Priority: -- → P2
Comment 1•18 years ago
|
||
Attachment #209886 -
Flags: review?(annie.sullivan)
Updated•18 years ago
|
Attachment #209886 -
Flags: review?(annie.sullivan) → review+
Comment 2•18 years ago
|
||
Comment on attachment 209886 [details] [diff] [review] Patch Oops, this patch was actually for bug 322290.
Attachment #209886 -
Attachment is obsolete: true
Assignee | ||
Comment 3•18 years ago
|
||
I think this has since been fixed.
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 2 alpha2
Assignee | ||
Comment 4•18 years ago
|
||
No it hasn't, although there's a new behavior: - the selection disappears, and nothing is added.
Comment 5•18 years ago
|
||
I've had the browser crash on my once when copying/pasting too. TB15095194Q
Assignee | ||
Comment 6•18 years ago
|
||
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #216367 -
Attachment is obsolete: true
Assignee | ||
Comment 8•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #216453 -
Flags: review?(annie.sullivan)
Comment 9•18 years ago
|
||
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-
Assignee | ||
Comment 10•18 years ago
|
||
(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?
Assignee | ||
Comment 11•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #216468 -
Flags: review?(annie.sullivan) → review+
Assignee | ||
Comment 12•18 years ago
|
||
Fixed, branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 13•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
•