afuna (![[personal profile]](https://www.dreamwidth.org/img/silk/identity/user.png)
afuna) wrote in ![[site community profile]](https://www.dreamwidth.org/img/comm_staff.png)
dw_dev_training2009-06-22 12:50 am
afuna) wrote in dw_dev_training2009-06-22 12:50 am
Entry tags:
Bug 1123 fix walkthrough: Post image link broken on update page
Walkthrough for Bug 1123 - Post image link broken on update page
Table of contents
RTE
Patch evaluation, clean up, submission
Getting reviewed, resubmitting
I picked up this bug, because I noticed it had been coming up frequently as an issue in Support. Long-term, the fix is to have our own image hosting setup (whoo!) but for now, it's easy enough to hide the tabs / sections that the site isn't set up to use.
By the way, if you're having trouble deciding picking out a bug to start, I'd suggest looking at the following:
So I go to Bugzilla, it's kinda a pain to have to type in my email and the status to assigned, so I use my Dreamwidth Bug Grabber Greasemonkey script, which lets me claim the bug with one click.
Likefoxfirefey, I use Mercurial Queues to manage my changes. My set up is slightly different, because I have a repository on my computer (local copy of the code) where I can edit the files using my graphical text editor, and then I push my changes to the dw-free repository on my server (server copy of the code), after which I move the changes over to my live site using
Before doing any coding, I need to make sure I'm working with the latest version of the code. I remove any patches I have applied, then I update my code:
Server still runs; good. I look at the bug report again to figure out what the bug is, and how to reproduce it (it's always good to double-check that updating didn't break your server, and that you can reproduce the bug before you start making any changes. That way, when things work/don't work later on, you'll know if it's your changes that caused the problem, because of what didn't work/was working when you started).
Luckily for me, the Support folks/beta testers have done a lot of the legwork. In particular,piranha commented with a bullet-point breakdown of the issue which tells me that I need to look at:
- the HTML editor: "Image from file" section under the Insert image tab; Photobucket tab
- the RTE: Upload tab, Photobucket tab
(The RTE doesn't make clear what "Link" is -- "Link" lets you wrap a link around the image so that clicking on it will bring you to that page, so you need to fill in the first tab, otherwise it will throw an error message. I've filed a bug for it, not fixing in this bug, as it falls outside the current bug's scope)
One reason I love using Mercurial Queues is that I can easily apply (push) and unapply (pop) my changes.
To view the changes to my dev site just after I make my changes, I do this:
To view my dev site without any changes, I do this:
To view my dev site with previous/saved changes, I do this:
So when I'm doing developing/testing, I tend to use
I decide to start with the HTML editor, because the RTE breaks my brain.
Well, I know the bug is on image insertion in the update page, and from past experience I knew that many of the pop-ups that the RTE/HTML editor opens are BML pages of their own, so I open /update, open my Firebug console to the Net tab (Firebug = great extension for web development/diagnostics for Firefox), which would show me any new pages that I tried to load, and click on the "Insert Image" link. That gives me a: "GET imgupload.bml", so I know that htdocs/imgupload.bml is the page I want; it is the page with all the logic for the insert image pop-up.
Aside: htdocs contains the pages that you see when you visit the site, so http://dreamwidth.org/update.bml can be found in htdocs/update.bml, http://s.dreamwidth.org/img/celerity/dw_logo_celerity_beta.png can be found in htdocs/img/celerity/dw_logo_celerity_beta.png, etc.
I start by putting an if-statement around the line of JS which tries to use $LJ::PHOTOBUCKET_JWIDGET_ID; this gets rid of a JS error, caused by the lack of the $LJ::PHOTOBUCKET_JWIDGET_ID in our configuration files. Below is the code that I changed: lines prepended with a "-" were removed; lines prepended with a "+" were added.
That's just the first step, though; I also need to put if-statements around other Photobucket-related things, because, otherwise we'll get other JS errors, as the page tries to look for the (now) non-existent
Not bothering to paste the rest of the changed ifs here, but after I'm finished, the display of the Photobucket tab can be triggered by setting a value for $LJ::PHOTOBUCKET_JWIDGET_ID in etc/config-private.pl. If that configuration variable is not defined, there should be no sign of Photobucket anywhere.
So I save my changes, then transfer my changes to the live site:
I want to skip straight ahead to removing the upload stuff, but I decide to first open up "Insert Image" just to check... and arrgh it's a mess! The HTML elements are all jumbled up together. Turns out I don't print out the necessary closing tags if we don't have the Photobucket ID defined:
I fix that, things look okay now. As a last step, I check the error console for any JavaScript errors, to make sure I didn't (again) put an if-statement around too much, or too little. I do this with no $LJ::PHOTOBUCKET_JWIDGET_ID (no PB tab, no JS errors), and with a dummy value set for $LJ::PHOTOBUCKET_JWIDGET_ID (has PB tab, no JS errors).
The cryptically named $LJ::FB_SITEROOT appears several times on the page, in areas which look related to file uploads. It helps to know that "Scrapbook" (on LJ) uses the "Fotobilder" (FB) software. And SITEROOT is just the basic URL where you can find the photo hosting site setup. So, just like with $LJ::PHOTOBUCKET_JWIDGET_ID, I feel that I can use the presence (or absence) of a value for $LJ::FB_SITEROOT to tell me whether the site is set up to use Scrapbook or not.
There's some refactoring work needed (I did part of this for Photobucket, but the more convenient example is for the photo upload, so!). The original code which printed some of the JS was:
where,
* qq{} == double quotes, which does exactly the same thing as '"', but we can't use the actual quote character, because of the type="text/javascript" inside.
* having curly braces inside the qq{} works, but only if the these curly braces are matching (have both the opening and the closing)
* that text contains some JS which should only be printed when Photobucket/Scrapbook is set up properly, which means I need to chop the JS into chunks, which means that the curly braces for the Javascript will no longer match within one chunk
All this means that I need an alternative delimiter for "qq{}"; I choose "qq##". I'm still thinking over whether there's a better way to do this. It seems so ugly, but my changes end up looking something like this:
All that refactoring actually ends up introducing a syntax error! I double-check what changes I had made, by doing
I do this three or four times, and I eventually find out what was failing: I have an extra quote ("), which caused a syntax error, making the imageupload popup fail to render *rueful grin*. At this point, I can revert all my changes (
I test it the same way that I was testing the Photobucket changes: look in JS console, test with a value set for the $LJ::FB_SITEROOT, test with no value set for $LJ::FB_SITEROOT. I find a JS error, because the file div no longer exists sometimes; I add an if-statement, no new errors crop up. I'm ready to move on.
I notice that when you click the "Insert Image" link, you first get a blank white box, while the "Insert Image" loads, and then the "Insert Image" form. This background white box is always 300px high, no matter the height of the form inside it.
Because I'd removed the file upload section, the current form is much smaller now, and the white space surrounding it is ugly! So first I play around with removing the background color -- it works, but means that now we have no indication that "Insert Image" popup is loading. I finally settle on giving a min-height to the form content within the box, to make it expand to fill up/cover up the bottom white space.
Other considerations, though not obvious in the bits I posted, I tend to update the style (spacing/quotes) for the lines I'm already touching as a matter of course.
I should have done this before, but I only realize I could have done this after I am done with the HTML editor changes. Based onwyntarvox's comment, I run a search for PHOTOBUCKET_JWIDGET_ID:
(
That gives me:
htdocs/imguploadrte.bml:84: $$head .= "var pb_affsite_id = $LJ::PHOTOBUCKET_JWIDGET_ID;</script>\n";
htdocs/imgupload.bml:3: $$head .= "var pb_affsite_id = $LJ::PHOTOBUCKET_JWIDGET_ID;</script>\n";
Easy enough! I've already done htdocs/imgupload.bml, so I open up htdocs/imguploadrte.bml, do my changes. Nothing happens here that I didn't already run into when I was doing the HTML editor -- either that, or it was so traumatic that I've blocked it from memory -- so I am not going to go into this.
Oh! Okay, the one interesting thing is that I wanted to be able to determine, automatically, whether we should show the Photobucket/Fotobilder tabs in the RTE. The problem with this is that the RTE configuration code is in a JS file, which has no access to Perl variables. So we need to somehow bridge the gap between the two.
What I settled on was this:
A similar technique can be used when trying to english-strip text that's defined in .js files. Not straightforward, but it works.
Since I use Mercurial Queues, my patch is available at $LJHOME/cvs/dw-free/.hg/patches/bug1123 (I name all my patches directly after the bug I'm working on. Makes it easy to find the relevant zilla item later). If you're editing from live, then you'll need to generate your patch using $LJHOME/bin/cvsreport.pl -l --diff > ~/patches/name.patch. At some point I guess I should try working on an entire bug using it, so that I can actually give proper instructions :)
Anyway, once you have a patch file, you need to copy it over to your computer so you can upload it through your browser as an attachment. It's better not to copy and paste your patch from your terminal screen into a text editor. Lots of things can go wrong, including text-wrapping issues, line-ending issues, missing lines, extra lines... any of which could cause your patch to fail to apply.
For Unix-based computers, you can use the scp (secure copy) command. An example, copying from my server to the ~/patches folder on my local computer: scp dh-afuna@afunamatata.com:~/dw/cvs/dw-free/.hg/patches/bug1123 ~/patches. If you can't get that to work, a quick workaround is to copy the patch to $LJHOME/htdocs, and then hit up that URL to download the patch.
Now is a good time to open up the patch, reread my changes quickly. Make sure I didn't leave in any embarrassing debug printlns, that no irrelevant changes snuck in, that my code conforms to the style manual. If I'd seen something I didn't like, I would have edited the code on my server, tested again (if applicable), regenerated my patch, and then re-downloaded it to my computer. I don't run into any last minute changes (this time!), so I can upload my patch now.
In uploading a finished patch, there are three things to select:
But meh, I always forget at least one of the things though, so I wrote another Greasemonkey script which selects the review?, commit?, patch checkboxes. In the rare instances when I don't want these to be checked, I can uncheck them before submitting. Which reminds me, I now submit the patch. YAY.
Now comes the hard part: waiting for a review.
I've done my investigation, I've made my changes, I've tested my changes, I've run into my share of embarrassing/frustrating errors, I've made yet more changes, I've tested my changes again and again and again, I've submitted my patch. Now I wait for someone to look at my code, check it over for sanity and style, and either throw it back to be tweaked, or give it the stamp of approval so it can be committed to the code base.
There are two reasons to get a patch thrown back for reworking: form and function. And, uh, lucky for us, this bug had both. I promise I didn't plan it that way when I started this write up ;-)
Form is generally covered by the Style Guide. Also covered are things like the logic working, but being written in a confusing manner (e.g., unless... else statements, multiple variables declared on the same line, etc). Generally, patches won't be thrown back for style alone. (unless it's really widespread or really confusing). However, a reviewer may make a note re: confusing or messy code while checking for logic/general function.
kareila pointed out that
To regenerate the patch I do the following:
Being reviewed for function could be reviewing for missing functionality, ways in which the implementation or the effect could be improved, checking that the code isn't doing the wrong thing, checking for any syntax errors, checking also for edge cases where the code could stop doing the right thing. Basically, at this point all my testing will have revealed any errors that are obvious to me; now it's time to let someone else poke at it, see if they turn up anything obvious to them.
In this case, I end up with too much (broken and unnecessary) functionality!
Mark points out that we don't need the Scrapbook code anymore, we've ripped out a lot of support in other places (enough to make it non-functional); we should rip this out entirely. I kept it around just in case any DW clone started up before we got our image hosting solution in place, but Mark makes a lot of sense! Also, ripping out things is always fun.
So I go through the steps above *points*, only this time I hunt down the if-statements that surround scrapbook-related code, and delete them all. I am pretty sure that removing these won't cause any problem, since I've tested the case where Scrapbook is disabled in configuration, so I only test very lightly -- just enough to make sure I haven't inadvertently introduced any syntax errors.
It goes through a last round of review with no problems. And I'm done.
That got long! *laughs* Next time, I'll try to look for something shorter, and maybe also more technical.
Table of contents
Preparation / Background
I picked up this bug, because I noticed it had been coming up frequently as an issue in Support. Long-term, the fix is to have our own image hosting setup (whoo!) but for now, it's easy enough to hide the tabs / sections that the site isn't set up to use.
By the way, if you're having trouble deciding picking out a bug to start, I'd suggest looking at the following:
- effort-minor bugs
- the launch blockers
- known issues from Support; (these are the things that the Support team has noticed users running into) you'll need to track down the bugs filed in Bugzilla
- known issues on the wiki (this is the list of most of the user-facing issues that we have)
So I go to Bugzilla, it's kinda a pain to have to type in my email and the status to assigned, so I use my Dreamwidth Bug Grabber Greasemonkey script, which lets me claim the bug with one click.
Like
foxfirefey, I use Mercurial Queues to manage my changes. My set up is slightly different, because I have a repository on my computer (local copy of the code) where I can edit the files using my graphical text editor, and then I push my changes to the dw-free repository on my server (server copy of the code), after which I move the changes over to my live site using bin/cvsreport.pl -s -c. For the sake of simplicity, I'll cut out the first part, and go through this walkthrough as if I'm editing the files in my server repository (~/dw/cvs/dw-free), and then transferring them to the live site.Before doing any coding, I need to make sure I'm working with the latest version of the code. I remove any patches I have applied, then I update my code:
hg qpop -a # remove all patches that are currently applied
hg pull -u # pull the changes for the dw-free repository; pretend I did the rest of the steps in Dev Maintenance as well
Server still runs; good. I look at the bug report again to figure out what the bug is, and how to reproduce it (it's always good to double-check that updating didn't break your server, and that you can reproduce the bug before you start making any changes. That way, when things work/don't work later on, you'll know if it's your changes that caused the problem, because of what didn't work/was working when you started).
Luckily for me, the Support folks/beta testers have done a lot of the legwork. In particular,
piranha commented with a bullet-point breakdown of the issue which tells me that I need to look at:- the HTML editor: "Image from file" section under the Insert image tab; Photobucket tab
- the RTE: Upload tab, Photobucket tab
(The RTE doesn't make clear what "Link" is -- "Link" lets you wrap a link around the image so that clicking on it will bring you to that page, so you need to fill in the first tab, otherwise it will throw an error message. I've filed a bug for it, not fixing in this bug, as it falls outside the current bug's scope)
(Interlude) Applying Changes
One reason I love using Mercurial Queues is that I can easily apply (push) and unapply (pop) my changes.
To view the changes to my dev site just after I make my changes, I do this:
cd $LJHOME/cvs/dw-free;
hg qrefresh; # save my changes to my current patch
$LJHOME/bin/cvsreport.pl -s -c # transfer the newly-modified files to the live site
To view my dev site without any changes, I do this:
cd $LJHOME/cvs/dw-free;
hg qpop -a; # remove all my changes
$LJHOME/bin/cvsreport.pl -s -c # transfer the now-clean files to the live site
To view my dev site with previous/saved changes, I do this:
cd $LJHOME/cvs/dw-free;
hg qpush; # add one patch / one set of changes
$LJHOME/bin/cvsreport.pl -s -c # transfer the newly-modified files to the live site
So when I'm doing developing/testing, I tend to use
hg qrefresh a lot. When I'm done testing, or when I want to double-check how something behaves without my changes, I hg qpop all my changes, bin/cvsreport.pl to move them live, and my live site code is nice and clean again.HTML editor
I decide to start with the HTML editor, because the RTE breaks my brain.
Finding stuff
Well, I know the bug is on image insertion in the update page, and from past experience I knew that many of the pop-ups that the RTE/HTML editor opens are BML pages of their own, so I open /update, open my Firebug console to the Net tab (Firebug = great extension for web development/diagnostics for Firefox), which would show me any new pages that I tried to load, and click on the "Insert Image" link. That gives me a: "GET imgupload.bml", so I know that htdocs/imgupload.bml is the page I want; it is the page with all the logic for the insert image pop-up.
Aside: htdocs contains the pages that you see when you visit the site, so http://dreamwidth.org/update.bml can be found in htdocs/update.bml, http://s.dreamwidth.org/img/celerity/dw_logo_celerity_beta.png can be found in htdocs/img/celerity/dw_logo_celerity_beta.png, etc.
Photobucket
I start by putting an if-statement around the line of JS which tries to use $LJ::PHOTOBUCKET_JWIDGET_ID; this gets rid of a JS error, caused by the lack of the $LJ::PHOTOBUCKET_JWIDGET_ID in our configuration files. Below is the code that I changed: lines prepended with a "-" were removed; lines prepended with a "+" were added.
- $$head .= "<script type='text/javascript'>var seedurl='$LJ::SITEROOT/photobucket_cb.bml';";
- $$head .= "var pb_affsite_id = $LJ::PHOTOBUCKET_JWIDGET_ID;</script>\n";
+ if ( $LJ::PHOTOBUCKET_JWIDGET_ID ) {
+ $$head .= "<script type='text/javascript'>var seedurl='$LJ::SITEROOT/photobucket_cb.bml';";
+ $$head .= "var pb_affsite_id = $LJ::PHOTOBUCKET_JWIDGET_ID;</script>\n";
+ }
That's just the first step, though; I also need to put if-statements around other Photobucket-related things, because, otherwise we'll get other JS errors, as the page tries to look for the (now) non-existent
seedurl and pb_affsite_id variables.Not bothering to paste the rest of the changed ifs here, but after I'm finished, the display of the Photobucket tab can be triggered by setting a value for $LJ::PHOTOBUCKET_JWIDGET_ID in etc/config-private.pl. If that configuration variable is not defined, there should be no sign of Photobucket anywhere.
So I save my changes, then transfer my changes to the live site:
(in vi)
:w
(from the commandline)
hg qrefresh;
$LJHOME/bin/cvsreport.pl -s -c
(open up the browser to test)
I want to skip straight ahead to removing the upload stuff, but I decide to first open up "Insert Image" just to check... and arrgh it's a mess! The HTML elements are all jumbled up together. Turns out I don't print out the necessary closing tags if we don't have the Photobucket ID defined:
- $ret .= "<li id='tabPhotobucket'><a href='javascript: void(0);' onclick='window.parent.InOb.photobucket(seedurl,pb_affsite_id);setTab(\"tabPhotobucket\");return false;'>Photobucket</a><li></ul></div>";
+ $ret .= "<li id='tabPhotobucket'><a href='javascript: void(0);' onclick='window.parent.InOb.photobucket(seedurl,pb_affsite_id);setTab(\"tabPhotobucket\");return false;'>Photobucket</a></li></ul></div>" if $LJ::PHOTOBUCKET_JWIDGET_ID;
I fix that, things look okay now. As a last step, I check the error console for any JavaScript errors, to make sure I didn't (again) put an if-statement around too much, or too little. I do this with no $LJ::PHOTOBUCKET_JWIDGET_ID (no PB tab, no JS errors), and with a dummy value set for $LJ::PHOTOBUCKET_JWIDGET_ID (has PB tab, no JS errors).
Upload from File
The cryptically named $LJ::FB_SITEROOT appears several times on the page, in areas which look related to file uploads. It helps to know that "Scrapbook" (on LJ) uses the "Fotobilder" (FB) software. And SITEROOT is just the basic URL where you can find the photo hosting site setup. So, just like with $LJ::PHOTOBUCKET_JWIDGET_ID, I feel that I can use the presence (or absence) of a value for $LJ::FB_SITEROOT to tell me whether the site is set up to use Scrapbook or not.
There's some refactoring work needed (I did part of this for Photobucket, but the more convenient example is for the photo upload, so!). The original code which printed some of the JS was:
$$head .= qq{<script type="text/javascript">
// lots of stuff here
function clearTabs() {
...
}
}
where,
* qq{} == double quotes, which does exactly the same thing as '"', but we can't use the actual quote character, because of the type="text/javascript" inside.
* having curly braces inside the qq{} works, but only if the these curly braces are matching (have both the opening and the closing)
* that text contains some JS which should only be printed when Photobucket/Scrapbook is set up properly, which means I need to chop the JS into chunks, which means that the curly braces for the Javascript will no longer match within one chunk
All this means that I need an alternative delimiter for "qq{}"; I choose "qq##". I'm still thinking over whether there's a better way to do this. It seems so ugly, but my changes end up looking something like this:
- $$head .= qq{
- <script type="text/javascript">
+ $$head .= qq#
+ <script type="text/javascript">#;
+ $$head .= qq#
var fileaction = '$LJ::FB_SITEROOT/interface/webupload';
- var fbroot = '$LJ::SITEROOT/__using/$LJ::FB_DOMAIN';
+ var fbroot = '$LJ::SITEROOT/__using/$LJ::FB_DOMAIN'; # if $LJ::FB_SITEROOT;
// some more stuff here
function clearTabs() {
- \$('tabInsert').className = '';
- \$('tabPhotobucket').className = '';
+ \$('tabInsert').className = ''; #;
+ $$head .= qq#
+ \$('tabPhotobucket').className = ''; # if $LJ::PHOTOBUCKET_JWIDGET_ID;
All that refactoring actually ends up introducing a syntax error! I double-check what changes I had made, by doing
hg qdiff, but I still fail to see the cause. So I end up finding the error the hard way (you can skip this bit):- removed all my changes to the code (see Applying Changes), pushed the now-clean versions live
- checked that it works (yup!)
- opened up .hg/patches/bug1123 (Mercurial Queue puts all my patches into .hg/patches; bug1123 is what I named my patch)
- manually transfered over my changes a bit at a time to htdocs/imgupload.bml
- save imgupload.bml, transfer my changes live using
bin/cvsreport.pl -s -c(but not doinghg qrefresh; I don't want to save this set of changes. If I save this set of changes, it'd override the ones I've saved in .hg/patches/bug1123, and I'd lose all my work) - check that it still works (yup! yup! nope!)
- repeat, starting from transferring over changes
I do this three or four times, and I eventually find out what was failing: I have an extra quote ("), which caused a syntax error, making the imageupload popup fail to render *rueful grin*. At this point, I can revert all my changes (
hg revert -all), reapply my patch (hg qpush), fix the issue (edit out the stray quote, save), and then resume editing.I test it the same way that I was testing the Photobucket changes: look in JS console, test with a value set for the $LJ::FB_SITEROOT, test with no value set for $LJ::FB_SITEROOT. I find a JS error, because the file div no longer exists sometimes; I add an if-statement, no new errors crop up. I'm ready to move on.
Other issues
I notice that when you click the "Insert Image" link, you first get a blank white box, while the "Insert Image" loads, and then the "Insert Image" form. This background white box is always 300px high, no matter the height of the form inside it.
Because I'd removed the file upload section, the current form is much smaller now, and the white space surrounding it is ugly! So first I play around with removing the background color -- it works, but means that now we have no indication that "Insert Image" popup is loading. I finally settle on giving a min-height to the form content within the box, to make it expand to fill up/cover up the bottom white space.
Other considerations, though not obvious in the bits I posted, I tend to update the style (spacing/quotes) for the lines I'm already touching as a matter of course.
RTE
Finding stuff
I should have done this before, but I only realize I could have done this after I am done with the HTML editor changes. Based on
wyntarvox's comment, I run a search for PHOTOBUCKET_JWIDGET_ID:
hg grep PHOTOBUCKET_JWIDGET_ID
(
hg grep is similar to "rgrep", except that it searches only the files within the current repository, and you don't need to specify which folders to search from. hg help grep for more).That gives me:
htdocs/imguploadrte.bml:84: $$head .= "var pb_affsite_id = $LJ::PHOTOBUCKET_JWIDGET_ID;</script>\n";
htdocs/imgupload.bml:3: $$head .= "var pb_affsite_id = $LJ::PHOTOBUCKET_JWIDGET_ID;</script>\n";
Easy enough! I've already done htdocs/imgupload.bml, so I open up htdocs/imguploadrte.bml, do my changes. Nothing happens here that I didn't already run into when I was doing the HTML editor -- either that, or it was so traumatic that I've blocked it from memory -- so I am not going to go into this.
Oh! Okay, the one interesting thing is that I wanted to be able to determine, automatically, whether we should show the Photobucket/Fotobilder tabs in the RTE. The problem with this is that the RTE configuration code is in a JS file, which has no access to Perl variables. So we need to somehow bridge the gap between the two.
What I settled on was this:
- Find a suitable Perl file, which prints out stuff into update.bml (in this case, that would be in weblib.pl). This will serve as the "bridge" between the JavaScript and Perl. I can then mix my JS and my Perl by printing out the JS as normal text with Perl
- Add a new JavaScript Object(); assign it values using a Perl variable, e.g.,
+ $ret .= qq{ + <script type="text/javascript"> + var SiteConfig = new Object(); + SiteConfig.ImagePhotobucket = $photobucket_is_setup; + </script> + } - since that script tag gets printed out into the <head> of update.bml,
SiteConfigis now visible to any JS running on that page. So now we can use the value we just set, from any JS file:(in htdocs/stc/fck/editor/dialog/fck_image/fck_image.js) -if ( FCKConfig.ImagePhotobucket) +if ( FCKConfig.ImagePhotobucket && top.SiteConfig.ImagePhotobucket ) dialog.AddTab( 'Photobucket', 'Photobucket' ) ;
A similar technique can be used when trying to english-strip text that's defined in .js files. Not straightforward, but it works.
Patch evaluation, clean up, submission
Since I use Mercurial Queues, my patch is available at $LJHOME/cvs/dw-free/.hg/patches/bug1123 (I name all my patches directly after the bug I'm working on. Makes it easy to find the relevant zilla item later). If you're editing from live, then you'll need to generate your patch using $LJHOME/bin/cvsreport.pl -l --diff > ~/patches/name.patch. At some point I guess I should try working on an entire bug using it, so that I can actually give proper instructions :)
Anyway, once you have a patch file, you need to copy it over to your computer so you can upload it through your browser as an attachment. It's better not to copy and paste your patch from your terminal screen into a text editor. Lots of things can go wrong, including text-wrapping issues, line-ending issues, missing lines, extra lines... any of which could cause your patch to fail to apply.
For Unix-based computers, you can use the scp (secure copy) command. An example, copying from my server to the ~/patches folder on my local computer: scp dh-afuna@afunamatata.com:~/dw/cvs/dw-free/.hg/patches/bug1123 ~/patches. If you can't get that to work, a quick workaround is to copy the patch to $LJHOME/htdocs, and then hit up that URL to download the patch.
Now is a good time to open up the patch, reread my changes quickly. Make sure I didn't leave in any embarrassing debug printlns, that no irrelevant changes snuck in, that my code conforms to the style manual. If I'd seen something I didn't like, I would have edited the code on my server, tested again (if applicable), regenerated my patch, and then re-downloaded it to my computer. I don't run into any last minute changes (this time!), so I can upload my patch now.
In uploading a finished patch, there are three things to select:
- review? flag- to mark it for someone to look over your code
- commit? flag - to make it clear that once your code has been reviewed, it is ready to be committed (this is not always true; sometimes you can just upload a draft of your code to get comments on parts of it, or to pass it back and forth between teammates. Also, sometimes the reviewer is not the committer.)
- patch checkbox - this is important! Marking it as a patch makes it plain text, which makes code reviewers' lives easier because we can open it in our web browser.
But meh, I always forget at least one of the things though, so I wrote another Greasemonkey script which selects the review?, commit?, patch checkboxes. In the rare instances when I don't want these to be checked, I can uncheck them before submitting. Which reminds me, I now submit the patch. YAY.
Now comes the hard part: waiting for a review.
Getting reviewed, resubmitting
I've done my investigation, I've made my changes, I've tested my changes, I've run into my share of embarrassing/frustrating errors, I've made yet more changes, I've tested my changes again and again and again, I've submitted my patch. Now I wait for someone to look at my code, check it over for sanity and style, and either throw it back to be tweaked, or give it the stamp of approval so it can be committed to the code base.
There are two reasons to get a patch thrown back for reworking: form and function. And, uh, lucky for us, this bug had both. I promise I didn't plan it that way when I started this write up ;-)
Form
Form is generally covered by the Style Guide. Also covered are things like the logic working, but being written in a confusing manner (e.g., unless... else statements, multiple variables declared on the same line, etc). Generally, patches won't be thrown back for style alone. (unless it's really widespread or really confusing). However, a reviewer may make a note re: confusing or messy code while checking for logic/general function.
kareila pointed out that qq## could be mistaken for a comment by a casual reader, which I agree with, so I regenerate my patch to fix that issue.To regenerate the patch I do the following:
- take my patch and reapply it to my repository. I could have re-downloaded the attachment and done a patch -p1 < bug1123.patch. Since I'm using MQ, though, I let it manage my patches for me, and (since the patch for bug1123 is at the top of my list of patches), I do this:
hg qpush# that takes the first patch from my list of patches, and applies it to my repository - make the changes. I open up htdocs/imgupload.bml, edit the delimiters, so that I'm using
qq~~, which at this point is the only delimiter I can think of which doesn't show up in JS and which won't be easily mistaken for anything else. Save my changes. - transfer my changes back to the live site
bin/cvsreport.pl -s -c - test my changes. Everything is still working!
- save my changes to my patch (actual regeneration step)
hg qrefresh - copy my patch to my computer; resubmit it
Function
Being reviewed for function could be reviewing for missing functionality, ways in which the implementation or the effect could be improved, checking that the code isn't doing the wrong thing, checking for any syntax errors, checking also for edge cases where the code could stop doing the right thing. Basically, at this point all my testing will have revealed any errors that are obvious to me; now it's time to let someone else poke at it, see if they turn up anything obvious to them.
In this case, I end up with too much (broken and unnecessary) functionality!
Mark points out that we don't need the Scrapbook code anymore, we've ripped out a lot of support in other places (enough to make it non-functional); we should rip this out entirely. I kept it around just in case any DW clone started up before we got our image hosting solution in place, but Mark makes a lot of sense! Also, ripping out things is always fun.
So I go through the steps above *points*, only this time I hunt down the if-statements that surround scrapbook-related code, and delete them all. I am pretty sure that removing these won't cause any problem, since I've tested the case where Scrapbook is disabled in configuration, so I only test very lightly -- just enough to make sure I haven't inadvertently introduced any syntax errors.
It goes through a last round of review with no problems. And I'm done.
That got long! *laughs* Next time, I'll try to look for something shorter, and maybe also more technical.