liv: Stylised sheep with blue, purple, pink horizontal stripes, and teacup brand, dreams of Dreamwidth (_support)
Liv ([personal profile] liv) wrote in [site community profile] dw_dev_training2010-07-18 09:19 pm
Entry tags:

Baby's first patch

So I was kind of an embryo dev for a long time, fixing little things in S2 and getting [personal profile] fu to turn my fixes into patches. I'm most proud of the first version of the Page Summary gadget and reworking custom reading page colors, including a mini-template thingy for layout makers. I decided I really ought to be making my own patches, which I viewed as graduating from embryo to baby. This turned out to be a bit of a saga, but now I've done it, I thought I'd contribute a bug write-up.

First of all I applied for a Dreamhack. This turned out to be incredibly easy; thanks so much, [personal profile] sophie. From then on, [personal profile] ninetydegrees' Windows Newbie Guide was totally my Bible. If you're as intimidated as I was by the concept of running a Dreamhack and making patches, fear not; she has made a complete step-by-step walkthrough which is incredibly clear and easy to follow. This kind of thing is why DW is so awesome! Thanks to the guide, I found that I could use PuTTY as a command line interface to my Dreamhack (which gave me a little warm glow as I know the guy who wrote it), and an awesome program called WinSCP which lets you look at and edit files in a GUI that looks a bit old versions of Windows Explorer. I did some setting up stuff as per the Newbie Guide; the only original thing I did was to set WinSCP to use my favourite text editor, Notepad++.

Then I ran away from world of Dreamhack for a while, and went back to my old friend the S2 layer editor. The bug I picked to dip my toes in was Add additional classes to Page::print_wrapper_start. That's a pretty simple bug (I thought), and only includes things that I already know how to do in S2.

So, I open up Core2. I search for Page::print_wrapper_start, since that's right there in the title of the bug. Here's the first problem: I can't figure out what this line does: var string class = $opts{"class"} ? """class="$opts{"class"}" """ : "";

I thought it might have something to do with making a list of different class names that we might want in the CSS for the body tag, but I wasn't sure what. I searched for $opts and didn't find it. I thought, well, Core2 works because of coding in S2.pm. I have no idea where that file is, but I poked around in the Wiki and eventually found another really useful walkthrough by [personal profile] ninetydegrees: Newbie Guide for patching styles, and that gave me a clue where to look: ~/dw/cvs/dw-free/cgi-bin/LJ. I searched for $opts in that (giant!) file, and found many instances of it, so I decided it was probably just a standard variable that means different things in different contexts. At this point I got a bit frustrated, and cried at my Beau, [personal profile] jack. He spotted that the code in Page::print_wrapper_start actually prints class="variable" inside another instance of class="", which doesn't work. We played around a bit, looking at the HTML source of various Dreamwidth journals, and decided that whatever $opts{"class"} was supposed to be doing, it was actually getting cleaned by the HTML cleaner and just printing a space in the <body> tag.

So I decided to ignore it and do the new classes in the same way that Core2 does the existing classes. I knew that the functions that decide who is looking at a journal page would be builtin functions, probably starting with viewer_is, so I searched for that in Core2 and found a nice list of relevant Boolean variables. Aha! I created a layout layer under Core2, and copied the Page::print_wrapper_start function to it. I then declared a bunch of variables which would print different CSS classes depending on whether the Boolean functions returned True. I expanded the <body> tag to include these variables, (and fixed the bit where $class was printing a spurious class="" statement). I created a style using just this function as a layout layer, and checked the HTML source to make sure that it was printing a <body> with all the appropriate classes. I tested by appending ?s2id=143661 to a bunch of journals, communities where I was or wasn't a member, personal journals where I was or wasn't subscribed or did or didn't have access, and convinced myself my code was doing what I expected.

I went into #dreamwidth-dev on IRC and asked [personal profile] exor674 for help with the $opts{"class"} issue. She was super-helpful, even though I wasn't very good at phrasing the question in the correct terms! So we agreed that that variable wasn't doing anything, and that my code worked even without using that bit, and she advised me to delete the line defining $class and just put $opts{"class"} inside the list of CSS classes in the <body> tag, in case someone ever wants to use it in the future.

Then I took a deep breath and went to Dreamhack to make a patch. I used the three previously created updating scripts to make sure my Dreamhack was up to date before I changed anything. Then I created a patch, using:
tocvs f
hg qnew -g bug824_pagewrapper_classes.diff


I used WinSCP to find ~/dw/cvs/dw-free/bin/upgrading/s2layers/core2.s2 and open it in Notepad++. I cut out the old Page::print_wrapper_start and replaced it with the code from my test layout layer, and saved the file (WinSCP saves stuff directly on the remote directory, making my life easier). I used dws followed by dwdb to make the changes apply on my Dreamhack. Then I looked at the Dreamhack and checked that things were still working when my changes were pasted into Core2 itself rather than a layout layer. (This shouldn't make any difference, but there can be errors in the copy/paste process!) I tried creating a custom CSS instruction that would make the page text red if the person viewing the page was the owner of the journal, and black otherwise, and tested that that worked.

I turned off my Dreamhack again, and used cvsreport.pl --sync to copy my changes from the CVS to the main version, then used hgqrefresh to turn the new code into a patch. And in WinSCP, I went to ~/dw/cvs/dw-free/.hg/patches and there was my bug824_pagewrapper_classes.diff file. I saved that to my desktop and then uploaded it to Bugzilla, including a note about the $opts{"class"} bug and what changes I'd made.

After several false starts (don't worry, I won't write up the saga of my horrendous attempts to make a color theme, which I'd expected would be an easy beginner bug and turned out to have hidden complications), it took me just a couple of hours from opening the bug to submitting a patch. OK, a couple of hours for 12 lines of code is not particularly impressive, but I was trying lots of things for the first time. I hope that as my babyhood phase progresses those effort-minor bugs will actually be a minor effort!

[personal profile] ex_wicker969 2010-07-19 04:22 am (UTC)(link)
This was really awesome/helpful, thank you.
heavenlyevil: Painting of a fairy croched menacingly on a flower. (Default)

[personal profile] heavenlyevil 2010-07-19 03:55 pm (UTC)(link)
Yes, very helpful!