Closed Bug 384800 Opened 17 years ago Closed 11 years ago

Improve prefpane development documentation

Categories

(Camino Graveyard :: Product Site, defect)

Product:
Camino Graveyard
Component:
Product Site
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: bugzilla-graveyard, Assigned: bugzilla-graveyard)

References

(
URL
)

Details

Attachments

(4 files, 6 obsolete files)

Placeholder to queue the wiki page
97 bytes, text/plain
mark
: review+
mark
: superreview+
Details
Branch crash log
29.17 KB, text/plain
Details
Updated Makefile support for building the CBSamplePane.zip
1.48 KB, patch
Details | Diff | Splinter Review
CBSamplePane that works on 10.3.9
15.65 KB, application/zip
Details 
We need to document the process of prefpane development. We've been talking about this for ages but we've never actually done it, although we have at least three devs who have written prefpanes and made them work.
Err... you mean this? 

http://wiki.caminobrowser.org/Development:Third-Party_prefPanes

This won't get done on www.cb.o, but it should get done on the wiki. I'm not sure we need a bug for this, just someone to do it. You volunteering?
Yes, that, except you know, actual instructions ;)

Froodian and I talked about this a bit last night and I think he's going to try to work on improving that page as he re-works MoreCamino. Most importantly, I think we need to document how to do it without inserting the prefpane as part of the Camino Xcode project (which AFAIK is how everyone has done it so far).

I'm quite willing to help him, and Ludo said that Geoff had something posted to the mailing list a while back and that he'd try to dig it up for me (I wasn't able to find anything on the ML myself, but Ludo said he thought it was from around June '04).

cl
Yes, the next person who does this needs to write docs (which means Ian, probably).

I know Geoff had "instructions" (there's a post in the forum from japser about it), but to my knowledge no-one other than Geoff ever had them.
URL: http://wiki.caminobrowser.org/Develop...
Summary: Document prefpane development on site → Improve prefpane development documentation
Ludo swore up and down last night that Geoff posted a prefpane project to the Camino ML back in '04, complete with source code and everything. I'll cc him to this bug as a reminder to check through his inbox for this (which he said he might still have around).

cl
Assignee: nobody → nobody
I did - but my mailbox did not contain it.
The pref example was hosted on geoff's site - and can't find it anymore. I've mailed him. no answers yet.
Did you guys asked julian if he had it ?
(In reply to comment #6)
> Did you guys asked julian if he had it ?
> 

I did, he does not. Jasper whiped his disk. So maybe BruceD, or John hicks ?
Someone who understands all this needs to update the Linking errors section to advocate use of bundle_loader for panes targetting only 1.6b1 and recent trunk or newer:

[2:36pm] mento: going forward, we should update that to use bundle_loader instead, it's better to catch link errors
Attached file sample project (obsolete) — 
OK, I've got a halfway decent rough draft up at the URL in the bug.

I've attached the sample project here for thorough scrutiny by mento and smokey; if there are other people who can help judge the example quality of the project, please add them as reviewers.
Attachment #294319 - Flags: review?(alqahira)
Comment on attachment 294319 [details]
sample project

Here are some of the issues I found, in no particular order:

1) Don't include the build/ dir in the source

2) CBSamplePanePref.nib is mis-named in the srcdir (kill the ~)

3) Please set the default window in CBSamplePanePref to use the default sizes as defined in http://wiki.caminobrowser.org/Development:Editing_Nibs (search for "595" if you can't find it)

4) You should probably set the Deployment Target in the nib to 10.3

5) Someone more knowledgeable than me should weigh in on this, but we should probably also set the "Release" configuration to use the right SDKs per arch.

6) I'm not really familiar enough with all those other build settings flags, so I don't know if the rest of the defaults are sane (and, again, we mostly only care about giving them good defaults for Release).

7) What's the "Build ResourceManager Resources" phase?  I see it's in our prefPanes, too, but it's empty there as well; is it needed?

8) Change the -bundle_loader default to /Applications/Camino.app....  It may not be where everyone's Camino is, but it is a sane default.

9) We might want to add a note in the tutorial about other things the developer should customize (InfoPlist.strings, other Info.plist items)?

10) Is it better to include a PreferencePaneBase.h that may become outdated vs. telling them to grab it from cvs or a release tarball?

mento, will you be sure to weigh in on my questions and "possiblies" above?
Attachment #294319 - Flags: review?(mark)
Attachment #294319 - Flags: review?(alqahira)
Attachment #294319 - Flags: review-
Actually, I suppose we want to land the sample pane in CVS so we can keep its settings and stub impls updated with changes, and then we'll just make a separate archive for it that we post on the website and replace periodically.

In that case, your tri-licenses are missing the 

 * The Original Code is __________________________________________.

and 

 * Portions created by the Initial Developer are Copyright (C) 2___
 * the Initial Developer. All Rights Reserved.

lines ;)  See http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c
(In reply to comment #11)
> (From update of attachment 294319 [details])
> Here are some of the issues I found, in no particular order:
> 
> 1) Don't include the build/ dir in the source

I could have sworn I got rid of that, but yeah, I'll make sure it's not there in the final zip file.

> 2) CBSamplePanePref.nib is mis-named in the srcdir (kill the ~)

Grrr. I blame IB for this. Easy fix.

> 3) Please set the default window in CBSamplePanePref to use the default sizes
> as defined in http://wiki.caminobrowser.org/Development:Editing_Nibs (search
> for "595" if you can't find it)

I did. Is that not showing up on your end? The width is set to 595, and the height is the actual height of the pane (in this case, 20px + 20px + however high a text field is).

> 4) You should probably set the Deployment Target in the nib to 10.3

I tried to do that about six different times, but IB kept setting it back to 10.5. Someone with a 10.4 toolchain is probably going to have to fix this unless someone can tell me how to make it work in IB3.

> 5) Someone more knowledgeable than me should weigh in on this, but we should
> probably also set the "Release" configuration to use the right SDKs per arch.

Mmm, yeah, good point. We probably should. :)

> 7) What's the "Build ResourceManager Resources" phase?  I see it's in our
> prefPanes, too, but it's empty there as well; is it needed?

I have no idea. Mento?

> 8) Change the -bundle_loader default to /Applications/Camino.app....  It may
> not be where everyone's Camino is, but it is a sane default.

Right, I don't keep a Camino there, so I keep forgetting to change this when I export the project. Worst-case scenario, this can be fixed by hand-editing the .pbxproj file's BUNDLE_LOADER flag.

> 9) We might want to add a note in the tutorial about other things the developer
> should customize (InfoPlist.strings, other Info.plist items)?

Yeah, probably a good idea.

> 10) Is it better to include a PreferencePaneBase.h that may become outdated vs.
> telling them to grab it from cvs or a release tarball?

...

(In reply to comment #12)
> Actually, I suppose we want to land the sample pane in CVS so we can keep its
> settings and stub impls updated with changes, and then we'll just make a
> separate archive for it that we post on the website and replace periodically.

Right. I think that's a better idea, although I don't believe PreferencePaneBase.h is really all that unstable. Before it moved to its present location, there had only been three changes to that file in its entire history aside from licence changes:

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/camino/PreferencePanes/Shared/PreferencePaneBase.h&root=/cvsroot

> In that case, your tri-licenses are missing the 
> 
>  * The Original Code is __________________________________________.
> 
> and 
> 
>  * Portions created by the Initial Developer are Copyright (C) 2___
>  * the Initial Developer. All Rights Reserved.
> 
> lines ;)  See http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

Yeah, that was originally intentional because I didn't anticipate this going in CVS, but if we're decided that it should, I'll put them back.

Version 2 of the sample project is going to be marginally more complex because I figured I'd give potential coders a teensy bit of sample code to work with as well, so I've changed the pref edited by the sample pane from the home page to keyword.enabled and keyword.URL. I may need some assistance with the nib files (IB3 really doesn't seem to like me for some reason) from someone with a 10.4 (or 10.3) toolchain.

cl
(In reply to comment #13)
> > 3) Please set the default window in CBSamplePanePref to use the default sizes
> > as defined in http://wiki.caminobrowser.org/Development:Editing_Nibs (search
> > for "595" if you can't find it)
> 
> I did. Is that not showing up on your end? The width is set to 595, and the
> height is the actual height of the pane (in this case, 20px + 20px + however
> high a text field is).

IB3 and IB 1.5 both agree(!) the dimensions are set to 591x368, with a min w/h of 214x10!

Over the break, Chris asked me to ask mento and smorgan to look over the wiki again: http://wiki.caminobrowser.org/Development:Third-Party_prefPanes

This attachment is just to get that task in their queues.
Attachment #295134 - Flags: superreview?(mark)
Attachment #295134 - Flags: review?(stuart.morgan)
Comment on attachment 295134 [details]
Placeholder to queue the wiki page

Good, needs a little bit of polish.  Almost there!

These instructions should have a reference to http://developer.apple.com/documentation/UserExperience/Conceptual/PreferencePanes/ somewhere.

You might also want to say somewhere that Camino preferences aren't all centralized in NSUserDefaults, and that preferences in Gecko's store are most easily accessed through the methods in PreferencePaneBase, such as getStringPref:withSuccess:, setPref:toString: and clearPref:.  It makes sense to say this down low, probably after the "Using Gecko functions and methods" section.  We should make it clear that we support setting and getting Gecko preferences as a first-class priority; some beginning users might be discouraged by what we say about calling into Gecko from panes.

>to be deployed on any version of Camino from 1.5 onward (thus requiring
>Mac OS X 10.3.9 or higher).

1.5 supports 10.3 (the .0 is implied).  Do we really need to say this in this document, though?

>Creating the Xcode project

You should recommend which template to select in Xcode.  I'm assuming you used the PreferencePane template, but it's kind of buried under "Standard Apple Plug-ins" when creating an Xcode project.

>You can change this in the Properties tab of Xcode's Target Info window
>(see screenshot).

Here, too, Xcode's got a lot of windows and menus and "things" going on - it's a good idea to hold hands of beginner-to-intermediate users and direct them to Project:Edit Active Target.  Also, with this sort of thing, I prefer clarity of operations over clarity of language, so it makes sense to say "Target Info window's Properties tab", because you've got to look at the window before you can pick the tab.

>please use our NIB Editing Guide

I think it's "nib", but for capitalization as a proper name in a title, call it a "Nib".

>If you plan to use any of Camino's methods, your preference pane should
>subclass Camino's PreferencePaneBase class.

Say that PreferencePaneBase is a subclass of the standard AppKit NSPreferencePane class, to make it clear to advanced users what they're getting.

>[Will bad things happen if the prefpane and the app both have a copy of
>PreferencePaneBase.mm compiled into them? -cl]

Minor warning spew to the console is about the worst that can be expected for now, and we had that all along until 1.6b1 anyway.  It's possible that if we change the internals of PreferencePaneBase in the future, panes that carry their own versions along won't work properly, but that seems like a stretch.  We need to maintain PreferencePaneBase as a stable API for obvious reasons, and we've gone to lengths to do so with the preference getters even before we really supported this stuff the right way with -bundle_loader.

>In order for the -bundle_loader switch to do its job,

You start talking about -bundle_loader without introducing it.  Why should I care about -bundle_loader?  This document may not even need to mention -bundle_loader by name, you might just be able to start this paragraph by saying that the linker needs to see Camino so that it knows what will be available when the preference pane is loaded.

>Using Gecko code in a third-party preference pane is somewhat more
>involved. You'll need to copy the proper headers (see above) and link
>against the appropriate libraries.

Instead of "using Gecko code in", how about "calling Gecko functions directly from"?

Gecko #includes are like spaghetti.  Instead of recommending adding them to the Copy Headers phase, it's probably a better idea to do what the preference panes in the in-tree build do, and set HEADER_SEARCH_PATHS to point to the relevant directories in a Gecko dist tree.  It's still annoying as hell, but this way, you only need to hunt down directories, and not individual headers.
Attachment #295134 - Flags: superreview?(mark) → superreview+
Comment on attachment 294319 [details]
sample project

The .zip contains .DS_Store files and resource forks.  They shouldn't be in there.

The .xcodeproj in here contains your .pbxuser and .mode* files.  Get rid of them, too.

Shave a few bytes off of the .tiff by preparing it the same way we prepare the ones we ship: run "tiffutil -cat CBSamplePanePref.tiff CBSamplePanePref.new.tiff" and move the new file over the old.

Drop the verison.plist file, nothing uses it.  It just identifies the template used to initially build the project.

When I build zips, I like to do them on the command line with "zip -r9DX dir.zip dir" - "man zip" for why.

All of this stuff named CBSamplePanePref.* sucks, I think it should be just CBSamplePane.*.  That's what you get from the Xcode template, I guess.  Can you rename them?  You'll need to hit the project file, the .h and .mm files, and Info.plist.

This reminds me: the guide should tell people to pick a custom name for their class, in addition to picking a custom name for their CFBundleIdentifier.  Class name conflicts can be just as bad.  That's why we name our classes OrgMozillaCaminoPreference*.  Recommend a similar naming scheme for third-party developers.

> 1) Don't include the build/ dir in the source

I don't see it in there.

> 5) Someone more knowledgeable than me should weigh in on this, but we
> should probably also set the "Release" configuration to use the right
> SDKs per arch.

Why only Release?

If you set the SDKs, set the deployment target, too.  We already talked about setting the compiler - not necessary unless you're making C++ calls across modules.  But note that anything with C++ built by gcc 4 won't run pre-10.3.9, while the developer may be expecting to produce a pane that runs on 10.3[.0].  This might deserve some thought.

> 7) What's the "Build ResourceManager Resources" phase?  I see it's in our
> prefPanes, too, but it's empty there as well; is it needed?


That's for .r files, to run Rez run on them to produce resource fork data.  You can get rid of it.

> 10) Is it better to include a PreferencePaneBase.h that may become
> outdated vs. telling them to grab it from cvs or a release tarball?

I think it's fine to include a PreferencePaneBase.h file - think of it as defining a "minimum interface".  We can add to it, but we can't subtract without breaking binary compatibility (which we don't want to do).  I like the later suggestion to keep this in CVS and cut a new zip file as needed.

Code:

CBSamplePanePref.mm:

Trailing blank line in mainViewDidLoad.

Some blank lines have whitespace on them because Xcode's editor sucks so much.  There's not too much to find by hand here, or you can use the sed I keep passing out to fix this.

"// other public methods go here" is after a private method?  Do we even need that comment?

You've mixed style (again, I blame the template):

- (void) mainViewDidLoad
- (NSString*)currentHomePage

Pick one, I prefer the latter.

CBSamplePanePref.h:

#import <PreferencePanes/NSPreferencePane.h>

This is going to come in from PreferencePaneBase.h anyway, so it's unnecessary.  (I guess this came out of the Apple template?  Our style would have been to use <PreferencePanes/PreferencePanes.h>.)

Trailing whitespace on the @interface line.

Project file:

I picked up on most of these by looking for things that were in boldface or that had <multiple values> displayed, indicating differences between release and debug settings.

ZeroLink is on in Target/Debug.  It's a pre-Xcode 3 default.  Turn it off.  It sucks.

I'd also prefer turning off Fix & Continue.  Generate Position-Dependent Code should be off for all targets.  You should click on "Instruction Scheduling" and press <delete> to take any values out of the project file.

In the project info, you should click on Prebinding and press <delete>.

You might want to set "Other Warning Flags" to -Wall (or -Wall -Werror) to encourage good practices.

The precompiled header thing probably slows such a simple build down.  Remove CBSamplePane_Prefix.pch from the project and the zip file and, turn off "Precompile Prefix Header", and empty out the value of "Prefix Header".

Change 2007 to 2008 in InfoPlist.strings!

I don't think we need to link directly against PreferencePanes.framework, that stuff comes in via PreferencePaneBase, which we're getting from Camino when it's defined as the -bundle_loader.

The code refers to textFieldHomePage, which I was expecting to be a text field in the nib, but the window in the nib is empty and there doesn't seem to be anything wired up to textFieldHomePage at all.  The nib claims that the owner is of class NSPreferencePane, but this should ideally be set to the name of our concrete implementation class, so that these connections can be made properly.
Attachment #294319 - Flags: review?(mark) → review-
Attached file v2, with cl's changes and a fixed nib (obsolete) — 
Chris asked me to fix his nib since he's in dev-tools-la-la-land.  I think it's fixed OK; it works, mostly.

I haven't looked over anything else, except that 

* Chris needs to list himself and his email address under the "Contributors" section in his code
* Do we want "The Original Code is Camino code." to read "The Original Code is CBSamplePane." ?
* I fixed the bundle_loader to use /Applications/Camino.app/Contents/MacOS/Camino
* What did we decide about SDK for PPC?
* There are some issues with FKA (because first-responder should be the text field, but the text field is not enabled if the checkbox is not checked, and in that case, the loop isn't getting set up)
* Technically, the text field pref should be indented, since it's dependant on the the checkbox.  That makes for an odd-looking nib.
* Changes to the text field do not auto-apply.


I didn't really look over anything else; I can do a re-review later if desired, but I just wanted to get Chris's changes + a (mostly?) working nib + these comments up.
Attachment #294319 - Attachment is obsolete: true
Why shouldn't the checkbox be the initial first responder?  In that case, if it's on, users with FKA off will still get the text field focused first.

In any event, _firstKeyView and _lastKeyView should be hooked up in the owner.
I don't know if we want to have every single Camino build create the archive, or if there's a better way to do this, but this does work, and I got to spend the evening playing with the Makefile.in productively ;)

This is obviously not an urgent review until we've got all of the nits worked out of Chris's code+project+nib combo, so get to it at your leisure.
Attachment #296090 - Flags: review?(mark)
Comment on attachment 296090 [details] [diff] [review]
Makefile support for building the CBSamplePane.zip

Er, I meant to \ those libs:: lines after I got the everything building properly, but I forgot and diffed before doing so.
(In reply to comment #19)
> Why shouldn't the checkbox be the initial first responder?  In that case, if
> it's on, users with FKA off will still get the text field focused first.
> 
> In any event, _firstKeyView and _lastKeyView should be hooked up in the owner.

Chris's original nib probably had it right (but it was an IB3-only nib, so I had to force-clear a bunch of stuff to be able to save it to IB2 format and then try to reconstruct it properly in IB2); I'm still flying blind at constructing nibs.

Er, and wrt to my Makefile changes, just so I don't spam the bug a third time:

1) I caught the extra && pwd I left in, too

2) In the zip lines (the one I added and the embed-replacements one)
>	cd CBSamplePane.tmp && $(ZIP) -r0DX ../../dist/CBSamplePane.zip \
>	  CBSamplePane/*
Why doesn't $(DIST) work in place of ../../dist ?
(In reply to comment #22)

> 2) In the zip lines (the one I added and the embed-replacements one)
> >	cd CBSamplePane.tmp && $(ZIP) -r0DX ../../dist/CBSamplePane.zip \

Why doesn't this use -r9DX? I haven't looked at the rest of the makefile; maybe that's what we do everywhere, but in light of comment 17 I thought it was interesting.
Status: NEW → ASSIGNED
Comment on attachment 295134 [details]
Placeholder to queue the wiki page

Asking for second round of review from Mark.
Attachment #295134 - Flags: review?(mark)
Comment on attachment 296090 [details] [diff] [review]
Makefile support for building the CBSamplePane.zip

> Why shouldn't the checkbox be the initial first responder?  In that case, if
> it's on, users with FKA off will still get the text field focused first.

No comments on this?

>Why doesn't $(DIST) work in place of ../../dist ?

$(DIST) is a relative path: $(DEPTH)/dist, where $(DEPTH) is .. in camino.  When you cd into CBSamplePane.tmp, $(DIST) will be taken as relative to that directory: objdir/camino/CBSamplePane.tmp/../dist, or objdir/camino/dist - this is wrong.  You can use ../../dist to get the right $(DIST), or ../$(DIST), same thing.  ../$(DIST) relies on $(DIST) remaining a camino-relative path.

>Why doesn't this use -r9DX? I haven't looked at the rest of the makefile;
>maybe that's what we do everywhere, but in light of comment 17 I thought it
>was interesting.

It should use -9 (best compression).  The -0 probably comes from where we zip up the embed-replacements stuff - like other mozilla zips, we don't use any compression there, so the app doesn't need to decompress stuff at runtime.  We just use zip as a handy container format.  We don't care about compression because the dimmidge is compressed.  For zips that will be distributed themselves, use -9.

CBSamplePane.tmp should be added to GARBAGE_DIRS.

>+	cd CBSamplePane.tmp && pwd && $(ZIP) -r0DX ../../dist/CBSamplePane.zip CBSamplePane/*

This should be fine if it zips CBSamplePane without the /*.  And get rid of the pwd, as you noted.

Are we missing a mkdir of CBSamplePane.tmp?  We want stuff staged in CBSamplePane.tmp/CBSamplePane, right?

I think that the zip building stuff should be its own target that's not part of the default build.  We can call the target CBSamplePane.zip.  Probably no need to put it into $(DIST), just leave it in objdir/camino.
Attachment #296090 - Flags: review?(mark) → review-
Attachment #295134 - Flags: review?(mark) → review+
I attempted to fix comment 19 here; otherwise, all the problems of comment 18 remain.

The nib is better, but if FKA is on and the checkbox is unchecked, I can't tab into the nib. :(
Attachment #296082 - Attachment is obsolete: true
(In reply to comment #25)

> CBSamplePane.tmp should be added to GARBAGE_DIRS.

Victim of late-night Makefiling; it was there and then got lost when I tried something else out ;)

> Are we missing a mkdir of CBSamplePane.tmp?  We want stuff staged in
> CBSamplePane.tmp/CBSamplePane, right?

Maybe?  As you've no doubt determined, I ended up copying the embed-replacement rule, and it doesn't do the mkdir (why? why not?).

> I think that the zip building stuff should be its own target that's not part of
> the default build.  We can call the target CBSamplePane.zip.  Probably no need
> to put it into $(DIST), just leave it in objdir/camino.

Isn't $(DIST) for final products/stuff that's intended to be distributed?  I'm not strongly against the objdir/camino, but given that's where we put final Camino apps and dimmidges, it seems like a sample prefPane distribution should end up there, too.
Comment on attachment 295134 [details]
Placeholder to queue the wiki page

> As with Firefox extensions, there are many opportunities for third-party 
> developers to step in and provide niche features or UI for things that Camino 
> doesn't provide by default.

This seems like a misleading thing to say, since third-party pref panes aren't even remotely as flexible as Firefox extensions.

> These instructions are presented in good faith, but we make no guarantees as
> to the stability of Camino's APIs (even across minor versions)

If this refers to the base pref pane class it's not true; we do keep compatibility versions of those methods. If it doesn't, I don't think we should use the term "API" at all.

> If you plan to use any of Camino's methods directly, your preference pane 
> should subclass Camino's PreferencePaneBase class

This "Using Camino methods" method needs to be very clear that using PreferencePaneBase is fine, and that using anything else is *not* a good idea, is very likely to break in a future version.

> You'll also need to copy over any Camino header files for methods 
> you're using in your preference pane.

The headers just need to be referenced in the project. There shouldn't even be a Copy Headers in pref panes.
Attachment #295134 - Flags: review?(stuart.morgan)
(In reply to comment #28)
> (From update of attachment 295134 [details])
> > As with Firefox extensions, there are many opportunities for third-party 
> > developers to step in and provide niche features or UI for things that Camino 
> > doesn't provide by default.
> 
> This seems like a misleading thing to say, since third-party pref panes aren't
> even remotely as flexible as Firefox extensions.

Suggestions, then?

> > These instructions are presented in good faith, but we make no guarantees as
> > to the stability of Camino's APIs (even across minor versions)
> 
> If this refers to the base pref pane class it's not true; we do keep
> compatibility versions of those methods. If it doesn't, I don't think we should
> use the term "API" at all.

Fixed.

> This "Using Camino methods" method needs to be very clear that using
> PreferencePaneBase is fine, and that using anything else is *not* a good idea,
> is very likely to break in a future version.

Fixed.

> > You'll also need to copy over any Camino header files for methods 
> > you're using in your preference pane.
> 
> The headers just need to be referenced in the project. There shouldn't even be
> a Copy Headers in pref panes.

Define "referenced", please. I know you could probably add the relevant folder to the header search paths, but the Copy Headers seems easier to explain and package for third-party use. It also seems better for this because the only Camino header they should be using is PreferencePaneBase.h, and we specifically discourage it for Gecko because, while it would work, it's really easy to miss something that Gecko needs with that rat's nest of #includes.

If we do decide to use header search paths instead, the sample project will need more work to remove the Copy Headers phase that's in it.

cl
sorry for coming in late if these had been addressed. i skimmed the comments but didn't see anything along these lines:

- the section of "Who should read this document?" doesn't actually answer that question until the very end of the paragraph. Even then, had a hard time. The bulk of the section explains why we need 3rd party developers and why we have an extension mechanism, but neither of those answer the question posed in the section header. Maybe have a line up front saying 3rd party developers who want to add functionality to camino, then have a 2nd paragraph explaining why.

- in "creating the xcode project" some of the important steps are buried within paragraphs of text (eg, changing the bundle id). it might be easier to follow if every step was laid out clearly with a bullet, like a checklist. Do these N things, boom boom boom, and you'll have a working project. Each bullet can have a bunch of explanatory text, but as you're following along with the webpage, it's easy to skip over a paragraph w/out realizing there was an action buried within it.

looks good overall, i like it. 
(In reply to comment #30)

> - the section of "Who should read this document?" doesn't actually answer that
> question until the very end of the paragraph. Even then, had a hard time. The

Yeah, good point. I've fixed this in the latest revision; see what you think.

> - in "creating the xcode project" some of the important steps are buried within
> paragraphs of text (eg, changing the bundle id). it might be easier to follow
> if every step was laid out clearly with a bullet, like a checklist. Do these N
> things, boom boom boom, and you'll have a working project. Each bullet can have
> a bunch of explanatory text, but as you're following along with the webpage,
> it's easy to skip over a paragraph w/out realizing there was an action buried
> within it.

Take a look at what I did there. I basically took what would have been bullet points and turned them into section subheaders. Now you can glance at the ToC now and get a good idea of what you need to do without even having to read the docs if you're familiar with preference panes in general.
Oh yeah, and just for the record here, I moved the page to

http://wiki.caminobrowser.org/Development:Third-Party_Preference_Panes

since I'm trying to get rid of this silly "prefpane" shorthand in official docs.
URL: http://wiki.caminobrowser.org/Develop... → http://wiki.caminobrowser.org/Develop...
My toolchain refuses to play nice with any of this. Someone else is going to have to figure it out.
Assignee: cl-bugs-new → samuel.sidler
Status: ASSIGNED → NEW
Assignee: samuel.sidler → nobody
(In reply to comment #18)

> * Chris needs to list himself and his email address under the "Contributors"
> section in his code

Done.

> * Do we want "The Original Code is Camino code." to read "The Original Code is
> CBSamplePane." ?

Still awaiting an answer, although it doesn't matter to me either way and this is easy enough to fix on check-in if it comes to that.

> * I fixed the bundle_loader to use
> /Applications/Camino.app/Contents/MacOS/Camino

Thanks. I ended up putting another copy of Camino on my hard drive in that location so I wouldn't have to deal with this while I work on it.

> * What did we decide about SDK for PPC?

Dunno. Mark or Stuart, do either of you have an opinion here?

> * There are some issues with FKA (because first-responder should be the text
> field, but the text field is not enabled if the checkbox is not checked, and in
> that case, the loop isn't getting set up)

I think I have this fixed in the new nib.

> * Technically, the text field pref should be indented, since it's dependant on
> the the checkbox.  That makes for an odd-looking nib.

Yes, it does, and I think we can overlook that minor detail since this is "just an example". (It also points out how tricky it is to do really good UI for this preference.)

> * Changes to the text field do not auto-apply.

After much gnashing of teeth and tearing of hair and rediscovering bug 411189 all over again, they do now.

New sample code, etc. coming shortly.
Hardware: Macintosh → All
Version: Trunk → unspecified
Attached file New sample project + accessories (obsolete) — 
Also updates PreferencePaneBase.h for the changes from bug 428858. (That's the only thing that changed in the file since this was last updated.)
Assignee: nobody → cl-bugs-new
Attachment #296296 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #323326 - Flags: review?(alqahira)
I have a minor update ready to go for the wiki whenever cb.o stops being totally unresponsive, too.
I know of at least one outstanding bug in the sample code (see comment 2 in bug 411189), but I'm waiting on an answer over there before I make a decision on what to do. Smokey, feel free to go ahead and test this out, with the caveat that changes made via about:config while the prefs window is open won't be properly applied.
(In reply to comment #37)
> what to do. Smokey, feel free to go ahead and test this out, with the caveat
> that changes made via about:config while the prefs window is open won't be
> properly applied.

Which is another bug, so it's not a big deal (but maybe we should mention that in the wiki docs?).

I also have an updated Makefile patch running around somewhere that addresses mento's coments on my last iteration....

I should get to this this week, though.

(In reply to comment #38)
> (In reply to comment #37)
> > what to do. Smokey, feel free to go ahead and test this out, with the caveat
> > that changes made via about:config while the prefs window is open won't be
> > properly applied.
> 
> Which is another bug, so it's not a big deal (but maybe we should mention that
> in the wiki docs?).

Well, it's another bug in the sense that if you follow the example of our current pref panes, you'll run into it, but there's no reason we should include that bug in CBSamplePane. I figure this may still need some other work and I didn't want to spam the bug with another attachment without also fixing the one bug I knew about.

cl
Comment on attachment 323326 [details]
New sample project + accessories

Three issues I've seen, from minor to major:

1) Make the NSHumanReadableCopyright in InfoPlist.strings match what we use elsewhere in Camino, or get rid of it (none of the prefPanes actually have that file).

2) Changes to the search URL don't auto-apply; they require you to select another pane first.

3) The prefPane crashes branch :(

Other than that, I think this looks OK.

My makefile patch (hmm, where did that go) packages the PPB header, so we won't need to commit that, but that's checkin fodder; it's easier to test with it in there.
Attachment #323326 - Flags: review?(alqahira) → review-
Attached file Branch crash log — 
This is from a debug CBSamplePane in attempt to get that first frame, but Release crashes as well, also with the missing first frame.
Comment on attachment 323326 [details]
New sample project + accessories

Actually, one more thing: do we need to set a Deployment Target to make a branch CBSamplePane compatible with 10.3.9?
(In reply to comment #27)
> (In reply to comment #25)
> 
> > Are we missing a mkdir of CBSamplePane.tmp?  We want stuff staged in
> > CBSamplePane.tmp/CBSamplePane, right?
> 
> Maybe?  As you've no doubt determined, I ended up copying the embed-replacement
> rule, and it doesn't do the mkdir (why? why not?).

There's still not one there; like embed-replacements, it doesn't seem to be needed, but if you tell me why we should have it (and why e-r also needs it or does not need it), I'll be happy to add it.

> > I think that the zip building stuff should be its own target that's not part of
> > the default build.  We can call the target CBSamplePane.zip.  Probably no need
> > to put it into $(DIST), just leave it in objdir/camino.
> 
> Isn't $(DIST) for final products/stuff that's intended to be distributed?  I'm
> not strongly against the objdir/camino, but given that's where we put final
> Camino apps and dimmidges, it seems like a sample prefPane distribution should
> end up there, too.

Target's there as requested, but the debate over $(DIST) remains ;)

Also, do we need to add some other --exclude things to cover .DS_Store, CVS/, *.mode*, *.pbxuser, and friends, or are they covered in some .cvsignore file I'm not following?
Attachment #296090 - Attachment is obsolete: true
Attachment #324245 - Flags: review?(mark)
That crash is a result of the changes from bug 428858. CBSamplePane's code was updated to work with the latest trunk prefs API, but now trunk and branch are different enough that the same code won't work both places. We need to either fix that or document it, bearing in mind that documenting it won't make deploying a working pref pane on both branch and trunk any less of a pain in the ass.

Basically, a dev would need to maintain two separate codebases and users would have to know whether they're installing the branch version of the pane or the trunk version of the pane. That doesn't strike me as particularly friendly to either devs or users, so my vote is that we land bug 428858 on branch.

I suppose we could also back it out on trunk, but it seems like a change we'll want to take eventually, and when we do take it, we'll break any preference panes written for the "old" API and be right back in this situation again.

Another option would be to tell third-party developers that pref panes will only ever work with 2.0 and up, but that doesn't strike me as a great idea either when we don't even have a 2.0a build out the door yet.
Attached file new CBSamplePane after fix for crash (obsolete) — 
Smokey, can you give this a try?

Bug 454710 has a fix for the crash in it, which I've compiled into this version of CBSamplePane. It works on both trunk and branch for me, but I'd like someone else to give it a shot too.
Attachment #338002 - Flags: review?(alqahira)
Comment on attachment 338002 [details]
new CBSamplePane after fix for crash

Yes, this pane seems to work fine for me on branch and trunk.
Attachment #338002 - Flags: review?(alqahira) → review+
Comment on attachment 338002 [details]
new CBSamplePane after fix for crash

Sticking this back in Smokey's queue for him to test on 10.3.9.
Attachment #338002 - Flags: review+ → review?(alqahira)
Comment on attachment 338002 [details]
new CBSamplePane after fix for crash

Yeah, this crashes and burns on open on branch on 10.3.9.

If I get a chance, I'll fiddle with the build settings.
Attachment #338002 - Flags: review?(alqahira)
Other stuff to document:

* the Gecko pref listener (see bug 384689 for a real-world example)
* notes about migrating prefs (maybe?)

I thought I had something else to add to this list but I can't think of it at the moment. Anybody else?
Where are we on this?  I thought at one point post-comment 48 we had a version that worked OK on branch?

At this point, though, I'm sorely tempted to simply have this be trunk-only and just release the package in conjunction with 2.0.
(In reply to comment #50)
> Where are we on this?  I thought at one point post-comment 48 we had a version
> that worked OK on branch?

We just don't have one that works on 10.3.9 yet. It works fine on 10.4+ with branch, or so you said in comment 46. :)

I've e-mailed my latest to Smokey and he's going to fool around with the build settings as promised in comment 48 to see if he can get it working on 10.3.9. Otherwise, I think I've done everything I can do to help this progress.
This appears to work now; I set the SDKROOT_ppc and _DEPLOYMENT_TARGET_ppc to 10.3 and the 10.3.9 SDK, respectively.

One thing I noted is that I can't get this to build in a side-by-side Xcode 2.5 install on 10.5.6; "$(DEVELOPER_SDK)/MacOSX10.4u.sdk" and friends fails because I have a space in my Xcode 2.5 path and Xcode 2.5 doesn't seem to quote it :/ and "/Developer/SDKs/MacOSX10.4u.sdk" and friends fails with some bizarre error about Metrowerks.  Someone needs to verify that it really does builds on a stand-alone Xcode 2.5 or 2.4.x install.

As before, this currently includes PreferencePaneBase.h for ease of testing, but we won't check that in.

(There's also the issue that we don't end editing when the pane loses focus, but that's an existing bug in our prefPanes.)

I think this (code + project) is probably ready for review now.
Attachment #323326 - Attachment is obsolete: true
Attachment #338002 - Attachment is obsolete: true
Attachment #354782 - Flags: review?
CBSamplePane project successfully builds with Xcode 2.5 on 10.4.11 PPC.
Comment on attachment 354782 [details]
CBSamplePane that works on 10.3.9

hendy said he was looking (or was going to look) at this code-wise, too, so tossing the review to him officially.
Attachment #354782 - Flags: review? → review?(trendyhendy2000)
Comment on attachment 324245 [details] [diff] [review]
Updated Makefile support for building the CBSamplePane.zip

I'm cancelling both of the reviews here, too.

We don't care about 10.3.9 (or 1.5 and 1.6) any more, so the project is obsolete.  With the project obsolete, the Makefile changes for packaging are irrelevant.

The wiki page also needs to be cleaned up for a world where 2.0/10.4 is the baseline.
Attachment #324245 - Flags: review?(mark)
Given the current state of the Camino project, we won't be fixing these website bugs. Mass changing our Product Site bugs (search on "camino-website-bugs").

RESOLVED -> WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: