karzilla: a green fist above the word SMASH! (Default)

Bug counts: resolved 367, assigned 10, needs-review 7

I managed to work on 7 "from-suggestions" bugs for the September hackathon. All were effort-minor: 2159, 2821, 2825, 2836, 2838, 3880, 3886.

I also submitted a walkthrough for 2159: http://dw-dev-training.dreamwidth.org/27698.html
karzilla: a green fist above the word SMASH! (Default)

Bug counts: resolved 244, assigned 16, needs-review 8

So the influx of new users from LJ pooh-poohing our themes got me in the mood to fiddle with styles. Which is to say, I dreamed about it all night Saturday and then poked around with CSS all day yesterday while totally ignoring my family and the mess they created in my house over the weekend, because that's how I roll.

Not that I have any grand illusions of being a web designer, mind you, but I can create a layer and I can do basic CSS overrides. That's closer to web coding than web design.

The comment I focused on was from a user (didn't save the link, alas) who wanted a layout with the journal-specific background color (you know, the one you can choose on the Manage Circle page) in a strip down the side of the entry on the user's reading page. This resonated with me because I have my own LJ reading page configured that way, so I tried to reproduce it here on DW. But since I didn't want to write a new layout from scratch (see also: not a web designer) I modified an existing theme to add the feature.

It's basically done at this point, unless I decide to muck around with CSS positioning some more trying to move the journal/poster info into the strip, instead of just the journal icon. But as it is, it's okay I think. I just need to pick out some distinctive colors that hopefully don't make people want to spork their eyes out, and then I'll submit it to [site community profile] dreamscapes, probably later today.

Soon I'll return you to your regularly scheduled backend coding arcana.

ETA: http://dreamscapes.dreamwidth.org/42503.html
karzilla: a green fist above the word SMASH! (Default)

Bug counts: resolved 199, assigned 11, needs-review 6

I hadn't been planning on doing any dev work today, but I saw bug 2749 come in and I couldn't resist. I had committed the patch that added AO3 to the list, and I had just been working on bug 733 which touched the same code in DW::External::Site, so I pounced on it.

My first thought was to explicitly disallow any site that didn't have a servicetype of "lj" since I knew we only crossposted to LJ-based sites. That's the same approach I used when dealing with detecting journaltype in 733. I knew I was making an assumption about protocols, and it nagged at me, but it produced the desired result.

However, [personal profile] alierak helped me figure out the better way to do it, which was to ask XPostProtocol for its protocol mappings. That would let me more authoritatively say "these are the sites that can crosspost." And when [personal profile] allen comes along and adds support for a different protocol, that list can update itself without any code changes.

So now in addition to the get_sites method that lists all the defined sites, we also have get_xpost_sites that only lists the sites that can crosspost. Yay!

As a bonus, I saw another optimization that could be made from XPostProtocol and copied it to Site - using LJ::ModuleLoader->autouse_subclasses instead of copying out the list of external site subclasses in the file. Again, one less thing to maintain manually.

Since this is a pretty big change to the header of the same code file I edited in 733, the patches aren't compatible. I pulled my 733 patch from the queue and will regenerate it once my new code from 2749 is approved.
karzilla: a green fist above the word SMASH! (Default)

Bug counts: resolved 198, assigned 11, needs-review 8

I spent some time today helping [personal profile] cesy with bug 2512, attempting to reproduce the icon browser from the quick reply form on the standard reply page.

My main issue with the initial patch was that it needed better grouping of similar things. That is to say, if the code looks like this:

do thing1 if can_do_it;
do thing2 if can_do_it;
do thing3 if can_do_it;

It's easier to read and maintain if we reformat it like so:

if (can_do_it) {
    do thing1;
    do thing2;
    do thing3;

I also said "OMG do we really need all those js files?" [personal profile] fu assured me that we do. Sigh.

I briefly went down a wrong path when I thought we only needed to load the button from ReplyPage.pm, but I later realized we needed to account for sitescheme comment pages as well, which use talkpost.bml and talkpost_do.bml. And we need to load the js files on each of those three different pages that call this function. Double sigh.

There was one last bug in the final submission that [personal profile] fu fixed, which is that we assumed $remote was assigned. It was, but lower down in the file than where we were checking for it.

This will definitely be a target for later refactoring, very similar to what I did for the quote button in bug 2693. Sometimes I have to remind myself not to jump on it right away. Get it working first; refactor it later. This one is more annoying than the quote button, though, because the quote button wasn't loading js libraries...
karzilla: a green fist above the word SMASH! (Default)

Bug counts: resolved 191, assigned 8, needs-review 7

Today's random cleanup project:

Most people want to print out dates in ISO format, but there's no standard way in the code to do this, so we see code that looks like what just went through [site community profile] changelog for bug 1561:
  my ( $sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $isdst ) = gmtime( $u->timecreate );
  my $journalcreated = sprintf( "%04d-%02d-%02d", $year+1900, $mon+1, $mday );

If there's one lesson I want you to take away from my first post in this journal, it is that code duplication makes me twitch. In this case, it's unlikely that we're ever going to need to update that code (barring some major apocalyptic event rewriting our dates to count from a year other than 1900), but it seems like a good idea to make this a function, just so that people have to write less code every time they want to do this.

I will call my new function LJ::mysql_date - because it is a direct analog to LJ::mysql_time, which already exists in ljtimeutil.pl and does the same thing, except including the time instead of just the date string. I think iso_date and iso_time would be better names, but I'm not prepared to rock the boat that much today.

So now the above code can be made much simpler:
  my $journalcreated = LJ::mysql_date( $u->timecreate, 1 );

The '1' indicates we are using gmtime instead of localtime, as mysql_time also does.

Having done that, another simplification presents itself. Every ID being calculated in this code starts with the exact same form "tag:$LJ::DOMAIN,$journalcreated:$u->{userid}" which is calculated from user data and the site constant $LJ::DOMAIN. That makes it a good candidate for a user method, instead of constructing the string from scratch everywhere it is needed. Furthermore, if we ever need to change the format again (as this bug just did), we can update it in one place.

I will call it $u->atomid and define it as a subroutine in LJ/User.pm, which is the large file where most of the user method declarations live. I will add it to Section 26: Syndication-Related Functions, since it is related to generating the user's atom feed. There's only one other function there at the moment!

[personal profile] kaisa did a great job in the original patch of changing the affected lines from the old format to the new format. I'm simply looking one step further, to say "this should have been packaged more neatly in the first place."

My bug for this work is 2738 and you can view the full patch there.
Page generated Sep. 21st, 2017 03:14 am
Powered by Dreamwidth Studios