liv: Stylised sheep with blue, purple, pink horizontal stripes, and teacup brand, dreams of Dreamwidth (sheeeep)
[personal profile] liv posting in [community profile] livredor
So this patch has a really unfortunate history; I started working on it last summer, I was all gung-ho enthusiastic about DW dev, felt comfortable with all the tools including GitHub, and was ready to have a go at delving into the actual code rather than what I've been doing up to now, which is just patching styles and themes. I struggled a bit with the learning curve, but I really felt like I was making progress. I had to redo the logic because it turned out that I'd misunderstood the spec of what I was supposed to be implementing, but that was fine, and helped me to improve my understanding of how DW works.

And then I got some review comments asking me to improve the appearance of the eventual HTML to be printed, but unfortunately that happened just at the beginning a few months of being really busy at work and abroad for a couple of weeks, which meant I lost momentum for actually fixing the cosmetic bits of the bug. Anyway I eventually managed to complete the required CSS fiddling, right in the middle of typhoon Haiyan, meaning that [personal profile] fu was understandably rather distracted from reviewing the tail end of low-priority patches, and I didn't exactly want to nag her about it, so my final version kind of languished until this week. During that time I didn't quite find the motivation to start a new bug, so by now I've forgotten most of the skills and knowledge I was starting to pick up last summer.

Anyway, the good news is that [personal profile] kareila did a Current Contributors post, where I saw that I hadn't committed anything since June last year. That prompted me to ask [personal profile] fu if she'd forgotten about the last bits of my patch, and she very kindly checked it over and merged it. The less good news is that I have picked just the moment to get back into dev when our bug tracker has died, making it hard to remember what was on my to-do list or search for bugs that will usefully stretch my meagre coding abilities. I don't want to let that block me, so maybe I'll go back to doing styles bugs for a bit, because at least they're documented in [site community profile] dreamscapes.

I'm a bit worried about this, really. Without a shadow of a doubt this is partly my fault; I could have prodded more, I could have got started on a different bug while I was waiting for code review, I could have documented stuff better rather than just waiting until final version was merged, which I kept hoping would be "soon". But I think it's also likely to be a problem for a lot of newbie devs, as evidenced in the Contributors list; only 5 people have committed code in 2014, and only 20 or so since I got derailed from dev work last summer.

There's an S2 search module which (optionally) prints a search form actually on journal pages, so if it's enabled you don't have to go to the person's profile to search their journal. And the bug, now lost in the demise of Bugzilla, was to add to that search form a checkbox for including comments in the search if that was an option. The conditional clause is what makes this more than a one line of HTML job! In general, search is allowed if the journal being displayed is paid, or if the person reading the free account journal is a paid user, or both (ie free users can't search within free journals). I initially thought that being allowed to search comments as well as entries was a paid privilege, so I wanted to check whether the reader had a paid account before I displayed the checkbox. But it turns out that what's actually a paid privilege is whether the comments get indexed in the search database in the first place, so in fact what matters is whether the journal being displayed is a paid account.

In many ways it's quite good that I had to redo the logic because my initial version was basically repeatedly checking privileges at several levels in a redundant way. The code for this is split over several files, something that I was only starting to get my head round by working on this bug. And that meant that I didn't initially realize I was doing things like, effectively asking, can this person search here? in the middle of a module which would only be printed at all if the person could in fact search there.

The first challenge was understanding the relationship between S2.pm and User.pm. I was sort of familiar with S2.pm, which basically defines methods that can be used by the S2 language in layout layers. What I didn't really understand was that S2.pm itself calls methods that are defined in User.pm. It took quite a bit of poking about to find that. I also didn't really understand the difference between a method and a function (I'm somewhat used to functions from S2), or the syntax for using the former. People, I think mainly [staff profile] mark, held my hand in IRC and I looked at other examples of code which calls methods, and I managed to come up with some syntax that works. This is one of the places I wish I'd made better notes cos at this point most of a year later I'm completely blurry about what a method actually is or how you use it in Perl.

Anyway, overall the concept is that core2.s2, written in S2, just says print_search_form($*text_module_search_btn);, and that calls the method print_search_form from S2.pm, which is written in Perl. print_search_form mostly involves defining a variable called $search_form, which has a bunch of HTML in it. I figured out by reading the code that you can put multiple lines of HTML within that single variable, it says:
my $search_form = '<div class="search-form">';
$search_form .= '<form method="post" action="">';
$search_form .= '<input class="search-box" type="text" name="query" maxlength="255">'


Etc. OK, so my $variable = ''; $variable .= ''; is useful to know. Though I am still a bit confused by / have forgotten what my is doing, I think it's defining a new variable for the first time? The point of putting this in a method rather than just hard-coding all the HTML is that you can put conditional commands in to determine which HTML gets printed out, which is exactly what I want to do: print an "include comments?" checkbox if searching comments is allowed, and just leave it out otherwise.

The next new skill I had to learn was how to do HTML forms, which I've never really understood. I ended up doing a lot of looking at the source of the search page external to journals. It turns out that the checkbox I was trying to include is defined by <input type='checkbox' name="with_comments" id="with_comments" /> I'm still a bit confused by how that causes DW to "know" that it should send a search request looking for text that matches all the comments on the journal being searched. When I tried to read tutorials about HTML forms they were all talking about running CGI on your server, so I formed a kind of hazy idea that that's approximately what DW is doing somewhere under the hood. And it doesn't really matter because I don't have to deal with that bit to patch this bug, I just have to create a checkbox with the appropriate name and id and trust the site to do the rest.

Deciding whether searching comments is allowed means calling a method which should be defined in User.pm, because "is this reader allowed to search comments in this journal?" isn't really an S2 issue, it's not part of how to display the journal. It needs to query the database about who is trying to do this action (the viewer) and the journal they're trying to do it in. I was conscious that the structure of DW is to have what's called user caps, which basically means what things different levels of accounts are allowed to do, eg how many icons they can have, whether they can post by email etc. And this should be configurable by individual DW installs. So I didn't want to just test, is journal being read paid? because that would hard-code the idea that free users' comments aren't indexed and paid and above users' comments are, whereas really it should be a configurable user cap. I more or less figured out how to do that, but I also agreed with some other devs that setting up a new user cap shouldn't really be part of this bug, so I compromised by creating a User.pm method that could later be altered to check the user cap instead of just directly checking paid status, and made sure that check only needed to be made once in the codebase. Depending how much can be salvaged from the charred corpse of Bugzilla, I might try to do the user cap bit as a separate patch later on.

Syntax for calling a method: if ( $ju->allow_comments_indexed ) {}. The $ju is to do with calling methods on the viewer and journal, which I never understood more than hazily but at least I now know that there is something to understand. The curly brackets contain the code that I want print_search_form to execute if the comments are indexed in the journal being viewed, in this case add the HTML for printing out a comments checkbox to $search_form

As for the method itself, it doesn't look like much but this represents quite a few hours of reading related bits of code and trying to cram new concepts into my brain:
sub allow_comments_indexed {

my ( $u ) = @_;
return 0 unless LJ::isu( $u );

# Comments are indexed in paid accounts only
return 1 if $u->is_paid;

# Otherwise comments aren't indexed
return 0;
}
The gibberish stuff at the beginning, my ( $u ) = @_;, is the bit I cobbled together from various related methods, script-kiddie stylee. I think what it's doing is asking the database questions about the journal being viewed. There's also $by which I think is the person trying to do the action, and $remote and $journal which come up a few times and seem to have similar roles in Perl syntax, so I should keep an eye out and try to understand exactly what they mean.

Anyway, here's what actually got committed: Bug 5041 Include comments search in S2 search module, which is the above plus creating a bunch of spans in the search form so that I could style it with CSS, and then some fairly crude CSS to make the search form reasonably non-ugly whether or not the tickybox is present and whether the form is displayed horizontally or vertically and how much space it has. For someone who spends a lot of time working on the pretty on DW I am a really bad web designer. But anyway, that bit was just fiddly, I pretty much already knew how to make each element its own CSS class or ID and then use trial and error until I get CSS that makes it look how I want it. But I am so very glad of the time we spent early in 2009 rationalizing the CSS for all the journal styles so that when I introduce a new feature I don't have to do this bit separately for every possible journal style on the site. And deeply grateful to [personal profile] ninetydegrees and [personal profile] momijizukamori for all their ongoing hard work in keeping that rational, consistent system in order in the five years since.
From:
Anonymous
OpenID
Identity URL: 
User
Account name:
Password:
If you don't have an account you can create one now.
Subject:
HTML doesn't work in the subject.

Message:

If you are unable to use this captcha for any reason, please contact us by email at support@dreamwidth.org


 
Links will be displayed as unclickable URLs to help prevent spam.

February 2016

S M T W T F S
 123456
78910111213
1415 1617181920
21222324252627
2829     

Most Popular Tags

Style Credit

Expand Cut Tags

No cut tags
Page generated Jul. 28th, 2017 06:53 am
Powered by Dreamwidth Studios