Dragging a URL Proxy Icon to the Home Toolbar Button should set the homepage to the dragged URL.

ASSIGNED
Assigned to

Status

enhancement
P3
normal
ASSIGNED
14 years ago
9 years ago

People

(Reporter: camino-bugs, Assigned: murph)

Tracking

Details

Attachments

(1 attachment)

17.66 KB, patch
samuel.sidler+old
: review+
Details | Diff | Splinter Review
Reporter

Description

14 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050615 Camino/0.9a1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050615 Camino/0.9a1

For more info see: http://www.tradetricks.org/archives/001322.html

Reproducible: Always

Steps to Reproduce:

Actual Results:  
URL returns to Location Field. No action taken.

Expected Results:  
Set the homepage to the dragged URL.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → Camino1.2

Updated

13 years ago
Whiteboard: [good first bug]

Comment 1

13 years ago
This may be a tricky hack as to my knowledge, there isn't any way to keep track of something that is dragged to a toolbar item.
You have to use a custom view.

Updated

13 years ago
Whiteboard: [good first bug]
Assignee

Comment 3

13 years ago
I'm close to having a complete patch for this, so I'm changing assignment to myself (hope that's alright..)

Also, as cl mentioned in #camino, dragging multiple URLs onto the button will probably have to be supported in the future.  Bug 189930 will enable tab sets to represent the home page, in which case this bug can be updated to support the multiple URLs.

I'm creating a reusable DragDestinationButton which can also solve bug 344044 and  permit us to add drop support to any other button.
Status: NEW → ASSIGNED
Assignee: mikepinkerton → camino
Status: ASSIGNED → NEW

Updated

13 years ago
Blocks: 344044
Assignee

Comment 4

13 years ago
I slipped up on bugzilla again and submitted a patch for this over on bug 344044 by accident..

Sorry!
By the way, I think I asked this once before on IRC, but I don't remember the answer, so I'm asking here: with the current work-in-progress patch for this bug, what actually happens in the case where multiple items are dropped on the Home icon?

I think the most logical behaviour would be to use the first one and ignore the remainder. Obviously, per comment 3, that'll have to change eventually, but I think it's important to have a sensible default.

cl
Assignee

Comment 6

13 years ago
(In reply to comment #5)
> By the way, I think I asked this once before on IRC, but I don't remember the
> answer, so I'm asking here: with the current work-in-progress patch for this
> bug, what actually happens in the case where multiple items are dropped on the
> Home icon?

cl: The current patch will, as you suspected, take only the first URL and just disregard any additional ones on the dragging pasteboard.

Also, I didn't forget about finishing this patch but have placed it on the back burner temporarily in order to finish up some more pressing bugs which are targeted at our 1.1 release.  I'll revisit this again soon and make sure your review comments are addressed and verify that it works smoothly.  Again, anyone interested can track the progress on bug 344044, which is where I'm submitting patches for both related enhancements.

I might submit separate RFE bug report about supporting the drop of multiple URLs to set the home tab set, and have it depend on bug 189930, just so it doesn't get overlooked when we can offer that functionality.
Status: NEW → ASSIGNED
(In reply to comment #6)

> cl: The current patch will, as you suspected, take only the first URL and just
> disregard any additional ones on the dragging pasteboard.

Thanks for clarifying that for me.

> Also, I didn't forget about finishing this patch but have placed it on the back
> burner temporarily in order to finish up some more pressing bugs which are
> targeted at our 1.1 release.

Yeah, I figured. No rush. :)

Feel free to put the new patch here, though, since putting a patch in the other bug was, by your own admission, a mistake. ;) (The dependency is still set up for that one depending on this one.)

> I might submit separate RFE bug report about supporting the drop of multiple
> URLs to set the home tab set, and have it depend on bug 189930, just so it
> doesn't get overlooked when we can offer that functionality.

That sounds like a good idea. CC me on the followup bug when you file it.
Assignee

Updated

13 years ago
Blocks: 367752
Assignee

Comment 8

12 years ago
Posted patch patchSplinter Review
Instead of my previous approach consisting of a target and custom selector, I now opted to use a delegate for DragDestinationButton's drag behavior.  In line with the usual rationale behind delegation, this avoids the requirement of a new subclass for each button.

Dropping multiple URLs on the new tab button will open all of them.  We can very easily do the same for setting the home page when that feature becomes available.

I also made some changes to our NSPasteboard+Utils category.  Some explanation for those:

> ++ (NSArray*)URLPasteboardTypes;

I figured this would be helpful and, if used throughout the application, allows us to update code in only one place should we suddenly discover another pasteboard type which contains URLs.

It makes the following much simpler:
[dragButton registerForDraggedTypes:[NSPasteboard URLPasteboardTypes]];

> -// Get the set of URLs and their corresponding titles from the clipboards
> +// Populates the supplied arrays with URLs and titles obtained from the
> +// receiver's data, both consisting of NSStrings.
>  ...
>  - (void) getURLs:(NSArray**)outUrls andTitles:(NSArray**)outTitles
 
This method returns an NSArray, not NSSet, so I removed such a reference in the comment.  I also made it explicit that both URLs and titles are NSStrings, which I had to scan the implementation to find out when using it.

Finally, I believe this patch exposes (not causes) two issues elsewhere in our code.  When the home page is set via a drop, the General (Navigation) pref pane tends to fall out of sync.  The other problem concerns dragging the proxy icon from the location AutoCompleteTextField.  The latter is because we only allow NSDragOperationGeneric from PageProxyIcon if the drag is local, and can be fixed with:

>    if (isLocal)
> -    return NSDragOperationGeneric;
> +    return NSDragOperationGeneric | NSDragOperationCopy;

I'll find out more about the preference pane and report these if necessary.

cl: I'm requesting you for the follow up review, since you were looking at this before (when I filed under bug 344044).  I'm not sure how much time you have though, so feel free to pass onto someone else.  Thanks.
Attachment #263551 - Flags: review?(cl-bugs)
Comment on attachment 263551 [details] [diff] [review]
patch

I'm not gonna get to this any time soon, so clearing review request for now. If this still hasn't been reviewed by the time bug 189930 and/or bug 287790 land, I'll give it another look.
Attachment #263551 - Flags: review?(cl-bugs) → review?
Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.
Target Milestone: Camino1.6 → ---
Comment on attachment 263551 [details] [diff] [review]
patch

By some miracle of $DEITY, this still applies with only fixing the hunk in the #includes (thank peeja for Toolbar script items bitrotting it).  So I played with it a little bit, and it looks pretty slick and works well.

The one thing I did notice is that the buttons don't handle cmd-drags of links from content (or bookmarks from the bar), whereas the bookmarks bar accepts cmd-drags of links from content.

I can't really review the code, but I caught these nits: 

>Index: src/browser/BrowserWindowController.mm

>   else if ([itemIdent isEqual:HomeToolbarItemIdentifier]) {
>+    // Setup this item to handle dropped URLs.
>+    DragDestinationImageButton* dragButton = [[[DragDestinationImageButton alloc] initWithFrame:NSMakeRect(0.,0.,32.,32.) 

>   else if ([itemIdent isEqual:NewTabToolbarItemIdentifier]) {
>+    // Setup this item to handle dropped URLs.
>+    DragDestinationImageButton* dragButton = [[[DragDestinationImageButton alloc] initWithFrame:NSMakeRect(0.,0.,32.,32.)


"Set up" is the verb ;)

This has been sitting for too long, so hopefully someone who reviews code can get on it soon.
Attachment #263551 - Flags: review?(cl-bugs)
Comment on attachment 263551 [details] [diff] [review]
patch

Nick said this morning he'd try to take a look at this :)
Attachment #263551 - Flags: review?(nick.kreeger)
Comment on attachment 263551 [details] [diff] [review]
patch

r=me but mento should look at this.

(Smokey's nits can be fixed on check-in.)
Attachment #263551 - Flags: review?(nick.kreeger)
Attachment #263551 - Flags: review?(mark)
Attachment #263551 - Flags: review?(cl-bugs)
Attachment #263551 - Flags: review?
Attachment #263551 - Flags: review+
We should be sure to spin a follow-up on the cmd-drags from content, unless they should be fixed here or someone says they're not possible at all.
Comment on attachment 263551 [details] [diff] [review]
patch

Clearing review request as this patch is very much bitrotted.

Murph, I know you're pretty busy these days; want me to update the patch for you?
Attachment #263551 - Flags: review?(mark)
You need to log in before you can comment on or make changes to this bug.