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

ASSIGNED

Status

--
enhancement
ASSIGNED
14 years ago
9 years ago

People

(Reporter: samuel.sidler+old, Assigned: bugzilla-graveyard)

Tracking

(Depends on: 1 bug)

Details

Attachments

(4 obsolete attachments)

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.
-> Enhancement.
Severity: minor → enhancement
Not sure how easy this is, but I'm targeting to 1.1 as it's not a deal breaker
for 1.0.
Target Milestone: --- → Camino1.1
*** Bug 314779 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

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

cl
Assignee: mikepinkerton → bugzilla
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

13 years ago
Created attachment 215443 [details] [diff] [review]
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
Attachment #215443 - Flags: review?(stuart.morgan)
(Assignee)

Comment 6

13 years ago
Created attachment 215444 [details]
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

13 years ago
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)];
Attachment #215443 - Flags: superreview-

Comment 8

13 years ago
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.
Attachment #215443 - Flags: review?(stuart.morgan) → review-

Comment 9

13 years ago
(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.
"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.
(Assignee)

Updated

13 years ago
Blocks: 189930
(Assignee)

Comment 11

13 years ago
Created attachment 223637 [details] [diff] [review]
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.
Attachment #215443 - Attachment is obsolete: true
Attachment #215444 - Attachment is obsolete: true
Attachment #223637 - Flags: review?(stuart.morgan)
(Assignee)

Comment 12

13 years ago
Created attachment 223638 [details]
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
Attachment #223638 - Flags: review?(stuart.morgan)
Comment on attachment 223638 [details]
zipped up nib file

Really fixing the MIME type this time :/
Attachment #223638 - Attachment is patch: false
Attachment #223638 - Attachment mime type: text/plain → application/zip
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.... :)
(Assignee)

Comment 15

13 years ago
(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
(Assignee)

Comment 16

13 years ago
(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

13 years ago
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

13 years ago
(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.
(Assignee)

Comment 19

13 years ago
(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
(Assignee)

Comment 20

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

Obsolete. See bug 189930 for current patch.
Attachment #223637 - Attachment is obsolete: true
Attachment #223637 - Flags: review?(stuart.morgan)
(Assignee)

Comment 21

13 years ago
Comment on attachment 223638 [details]
zipped up nib file

Obsolete. See bug 189930 for current nib.
Attachment #223638 - Attachment is obsolete: true
Attachment #223638 - Flags: review?(stuart.morgan)
(Assignee)

Updated

13 years ago
No longer blocks: 189930
Depends on: 189930
(Assignee)

Comment 22

13 years ago
(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
QA Contact: preferences
Target Milestone: Camino1.1 → Camino1.2
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 → ---
You need to log in before you can comment on or make changes to this bug.