287790 - Add "Set to Current Page" button in Home Page Pref

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Preferences, enhancement)

Description

[Samuel Sidler (old account; do not CC)]

Most browsers have an option to set the current page the user is on as the home
page. Camino, however, doesn't. Let's add one.

Comment 1

[Wevah (Nate Weaver)]

-> Enhancement.

Comment 2

[Samuel Sidler (old account; do not CC)]

Not sure how easy this is, but I'm targeting to 1.1 as it's not a deal breaker
for 1.0.

Comment 3

[Samuel Sidler (old account; do not CC)]

*** Bug 314779 has been marked as a duplicate of this bug. ***

Comment 4

[Chris Lawson (gone)]

Taking. I figure as long as I'm messing around in there I should probably fix this too.

cl

Comment 5

[Chris Lawson (gone)]

Attachment: adds "Use Current Page" button to General pref pane

This patch does *NOT* include the nib change; that's coming in a zip file just for completeness (even though I don't anticipate that my nib will be checked in).

Worth noting that the nib has some nextKeyView linkage problems that should be fixed (and are fixed in my version).

This also fixes a minor warning in BrowserWindowController.mm for missing casts.

cl

Comment 6

[Chris Lawson (gone)]

Attachment: nib file in zip format

This nib has all the nextKeyView stuff fixed; check it in if you want, or tweak it, or fix the version in CVS. Doesn't matter to me :)

cl

Comment 7

[Simon Fraser [no longer active]]

Comment on attachment 215443 [details] [diff] [review]
adds "Use Current Page" button to General pref pane

>Index: mozilla/camino/src/browser/BrowserWindowController.mm
>===================================================================

>   nsCOMPtr<nsIPref> pref(do_GetService(NS_PREF_CONTRACTID));
>   if ( pref )
>     pref->UnregisterCallback(gTabBarVisiblePref, TabBarVisiblePrefChangedCallback, self);
>   
>   // Tell the BrowserTabView the window is closed
>   [mTabBrowser windowClosed];
>   
>+  // Post notification that we're closing so the "Use Current Page" button in general prefs
>+  // doesn't stay enabled if we're the last BWC
>+  [[NSNotificationCenter defaultCenter] postNotificationName:@"BWCDidCloseNotification" object:nil];

Any reason not to have the prefs just listen for NSWindow's NSWindowWillCloseNotification
notification? It doesn't really matter if it gets that for other windows.

>Index: mozilla/camino/PreferencePanes/Navigation/Navigation.mm
>===================================================================

> - (void)mainViewDidLoad
> {
>   if (!mPrefService)
>     return;
> 
>+  // needed to validate "Use Current Page" button each time prefs comes to front
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(windowDidBecomeMain:)
>+                                               name:NSWindowDidBecomeMainNotification
>+                                             object:nil];
>+
>+  // or when a browser window is closed
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(waitForEventAndValidateButton:)
>+                                               name:@"BWCDidCloseNotification"
>+                                             object:nil];

Use NSWindowWillCloseNotification as mentioned above.

>+- (void)didUnselect
> {

>+  [[NSNotificationCenter defaultCenter] removeObserver:self];

I don't think this is the right place to unregister, since you only register
for notifications when the nib is loaded first time.

>+- (IBAction)setCurrentPageAsHomePage:(id)sender
>+{
>+  BrowserWindowController* bwc = [[(MainController*)[NSApp delegate] getFrontmostBrowserWindow] windowController];

Hmm. How is it that we can call into application code from the bundle for the prefs pane without linking against something?

>+// needed because validation occurs before BWC posts closure notification otherwise
>+- (void)waitForEventAndValidateButton:(NSNotification *)aNotification
>+{
>+  [self performSelectorOnMainThread:@selector(validateUseCurrentPageButton) withObject:nil waitUntilDone:NO];

performSelector:withObject:afterDelay would be marginally better.

>+- (void)validateUseCurrentPageButton
>+{
>+  BrowserWindowController* bwc = [[(MainController*)[NSApp delegate] getFrontmostBrowserWindow] windowController];
>+  if (!bwc)
>+    [buttonSetCurrentPageAsHomePage setEnabled:NO];
>+  else
>+    [buttonSetCurrentPageAsHomePage setEnabled:YES];

[buttonSetCurrentPageAsHomePage setEnabled:(bwc != nil)];

Comment 8

[Stuart Morgan]

Comment on attachment 215443 [details] [diff] [review]
adds "Use Current Page" button to General pref pane

(In reply to comment #7)
> Any reason not to have the prefs just listen for NSWindow's
> NSWindowWillCloseNotification

Simon stole some of my review right out of my mouth ;)

> I don't think this is the right place to unregister, since you only register
> for notifications when the nib is loaded first time.

I think what you actually want to do is move the registration so that you are using the didSelect/didUnselect pair

But I have some other issues too:

You don't re-validate on selecting the pane, so if someone opens prefs, changes to another pane, closes their windows, and switches back, the button will be wrong.

>+- (void)windowDidBecomeMain:(NSNotification *)aNotification

This name makes my skin crawl.  It looks like the delegate function, but there isn't a delegate relationship, and the parameter is different.  I'd rather see this have another name.

>+  // needed to validate "Use Current Page" button each time prefs comes to front
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(windowDidBecomeMain:)
>+                                               name:NSWindowDidBecomeMainNotification
>+                                             object:nil];

This is actually revalidating the button every time *any* window comes to the front.  If someone leaves their prefs window open in the background, that seems like a lot of work being done for no reason.

I don't see an obvious way for a pane to get the enclosing window, but there is a way to get around this (and I can't decide if this is a nasty hack, or exactly what it was intended for--Simon?): there's a MVPreferencesWindowNotification posted containing the pref window whenever a new pref pane is loaded in.  You can subscribe to that in your load function, and have that in turn do the registration for NSWindowDidBecomeMainNotifications, but with:
object:[[aNotification userInfo] objectForKey:@"window"]
so you only get the pref window.

Comment 9

[Simon Fraser [no longer active]]

(In reply to comment #8)

> I don't see an obvious way for a pane to get the enclosing window,

You can call -window on any view that you happen to have an outlet to.

Comment 10

[Mike Pinkerton (not reading bugmail)]

"Hmm. How is it that we can call into application code from the bundle for the
prefs pane without linking against something?"

because it's all messages sent at runtime. There's no static code that need to be linked against.

Comment 11

[Chris Lawson (gone)]

Attachment: work in progress

This isn't finished, but I'd like to know if I'm going the right direction before continuing. I'm hoping to fix this and bug 189930 in one fell swoop, and the two are closely related.

Comment 12

[Chris Lawson (gone)]

Attachment: zipped up nib file

Here's the nib file.

Anyone who is not Stuart, feel free to give commentary as well.

UI comments should probably go in bug 189930, since the UI for this bug has pretty much already been decided.

cl

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 223638 [details]
zipped up nib file

Really fixing the MIME type this time :/

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

My one comment on the current state of this patch/nib combo is that, unlike all of our other prefs (except fonts, and there's a bug filed on that), this doesn't auto-apply; you have to switch prefPanes or close the prefs to apply it.

Hmm, it seems we don't do that right now anyway; is there a reason we don't?

I can imagine someone clicking the button and then immediately opening a new window to test this new feature.... :)

Comment 15

[Chris Lawson (gone)]

(In reply to comment #14)
> My one comment on the current state of this patch/nib combo is that, unlike all
> of our other prefs (except fonts, and there's a bug filed on that), this
> doesn't auto-apply; you have to switch prefPanes or close the prefs to apply
> it.
> 
> Hmm, it seems we don't do that right now anyway; is there a reason we don't?
> 
> I can imagine someone clicking the button and then immediately opening a new
> window to test this new feature.... :)

There's no real reason we don't commit the edit right away, but we're going to have to (I just haven't put the code in yet) when we support multiple pages, since writing out the string value of the text field isn't going to work any more in that case.

cl

Comment 16

[Chris Lawson (gone)]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > I don't see an obvious way for a pane to get the enclosing window,
> 
> You can call -window on any view that you happen to have an outlet to.
> 

Like [[self mainView] window]. :)

Which is what I just did for that (not reflected in the "work in progress" patch).

cl

Comment 17

[Håkan Waara]

Comment on attachment 223637 [details] [diff] [review]
work in progress

Can the BWC ever realistically be nil? If not, I think all that validation and notification code is a bit overkill.

Comment 18

[Stuart Morgan]

(In reply to comment #16)
> Like [[self mainView] window]. :)

I thought that's what I was trying that made me think there wasn't a good way to get the window; IIRC mainView's window was nil for some reason (in which case you'd want to use one of the controls you have, rather than mainView).  Was it working for you?

(In reply to comment #17)
> Can the BWC ever realistically be nil? If not, I think all that validation and
> notification code is a bit overkill.

Sure.  Close all your windows, and open prefs.

Comment 19

[Chris Lawson (gone)]

(In reply to comment #18)
> (In reply to comment #16)
> > Like [[self mainView] window]. :)
> 
> I thought that's what I was trying that made me think there wasn't a good way
> to get the window; IIRC mainView's window was nil for some reason (in which
> case you'd want to use one of the controls you have, rather than mainView). 
> Was it working for you?

Yeah, it seems to work fine. The validation is run every time the prefs window becomes the main window, but not when other windows become the main window, which is exactly what we wanted to happen.

cl

Comment 20

[Chris Lawson (gone)]

Comment on attachment 223637 [details] [diff] [review]
work in progress

Obsolete. See bug 189930 for current patch.

Comment 21

[Chris Lawson (gone)]

Comment on attachment 223638 [details]
zipped up nib file

Obsolete. See bug 189930 for current nib.

Comment 22

[Chris Lawson (gone)]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > Like [[self mainView] window]. :)
> > 
> > I thought that's what I was trying that made me think there wasn't a good way
> > to get the window; IIRC mainView's window was nil for some reason (in which
> > case you'd want to use one of the controls you have, rather than mainView). 
> > Was it working for you?
> 
> Yeah, it seems to work fine. The validation is run every time the prefs window
> becomes the main window, but not when other windows become the main window,
> which is exactly what we wanted to happen.

For the record, I checked, and this actually *didn't* work. Using a control's view's window worked, but then I discovered this lovely little utility method we inherited from somewhere, |didActivate|, and just implemented that instead. :)

cl

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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*.