[01:21] <wgrant> cprov, cjohnston: people.canonical.com/~wgrant/launchpad/almost-final‽.png
[01:22] <cprov> wgrant: hey hey
[01:24] <cprov> it is coming up very nicely and since the new layout is more flexible (div) it will be easier to increment.
[01:24] <wgrant> I hope so.
[01:25] <cprov> wgrant: I've just opened an asana task about finding a way to materialise the IC threads, specially in emails
[01:25] <wgrant> I've also dropped the "draft" and "publish" terminology. It nows says "Unsaved comment", and then "Include X inline comments" next to the "Save Comment" button.
[01:25] <wgrant> Yeah, I've been wondering about how to do that.
[01:25] <wgrant> It's a bit awkward, as they're all at different depths potentially.
[01:25] <cprov> wgrant: it's annoying to to read the reviews right now
[01:27] <cjohnston> looks good wgrant
[01:27] <wgrant> Someone want to review? https://code.launchpad.net/~wgrant/launchpad/ic-js-cleanup/+merge/219531
[01:28] <cprov> wgrant: I will
[01:29] <cprov> wgrant: can you review https://code.launchpad.net/~cprov/launchpad/delete-branches-with-ics ?
[01:30] <wgrant> Hm
[01:30] <wgrant> What do you guys think about renaming "inline" to "diff"?
[01:30] <wgrant> That pretty much just affects the "Include X inline comments" checkbox
[01:31] <wgrant> Makes it clearer what it refers to.
[01:31] <cprov> wgrant: I wonder if I can just add few other 'ouch' in your MP and let you figure them out ;-)
[01:31] <wgrant> Heh
[01:31] <cjohnston> +1, more ouch
[01:31] <cprov> wgrant: +1 on "diff comments"
[01:32] <cjohnston> if you do that you should probably do the same in the email
[01:32] <cjohnston> what about 'View inline comments'
[01:32] <wgrant> Oh yeah, email and link too, forgot those.
[01:33] <cjohnston> I would argue that 'regular' comments are diff comments too..
[01:33] <cjohnston> IC is actually inline
[01:34] <cprov> wgrant: table-layout: fixed, not only predictable but also faster/easier for browsers. Nice one!
[01:35] <cprov> had to look it up ...
[01:36] <wgrant> It's pretty useful here, since divs expand to fill their container, and tds expand to fit their contents.
[01:38] <cprov> wgrant: what about the TextWidget for review_type ? what does it bring ?
[01:38] <wgrant> cprov: I narrowed it from 20 characters to 15, to ensure that the widgets all fit on one line.
[01:39] <cprov> ah, cool
[01:45] <wgrant> cprov: The rename is pushed.
[01:54] <cprov> wgrant: what does the CSS added in the BMP template do ?
[01:55] <wgrant> cprov: That's a variation on cjohnston's. It adds a bit of space before the publish checkbox.
[01:55] <wgrant> Also, I've just pushed a regression fix for something that seems to have broken a few days ago: the publish checkbox was being added unconditionally, so inline comments crashed and failed to load for anonymous users.
[01:57] <cprov> I think I had fixed that in 17011, on top of cjohnston fixes ...
[01:57] <wgrant> Oh, so you did
[01:58] <wgrant> I'll uncommit and merge
[02:02] <wgrant> cprov: Have you seen any more issues on (qa)staging since those firewall fixes a few hours ago?
[02:02] <cprov> wgrant: wait, there were 2 problems, you have to manually merge the conflicting hunks. I've only fixed the checkbox being rendered multiple times (in tests, essentially) and you have fixed the not-logged-in issue.
[02:03] <wgrant> Also, you added a tab!
[02:03] <wgrant> Die
[02:08] <cprov> I must, I don't know why the emacs in my vm is allowing this shit ... I will review my config
[02:09] <cprov> jslint should have exploded in my face when I try to commit, that's how we should setup project for dummies (like me)
[02:10] <wgrant> Heh
[02:15] <wgrant> cprov: The line-no col is wide enough to contain five digits plus the add sprite.
[02:17] <cprov> up to 99999 lines diffs then, not at problem
[02:17] <wgrant> Well, if you go back to your old days... :P
[02:17] <cprov> wgrant: so, you really want new methods on CRICSet ...
[02:18] <wgrant> cprov: All the other manipulation code (apart from person merge) is in codereviewinlinecomment.py
[02:18] <wgrant> I really don't want it to get like various other LP model classes, where there are ten different modules that alter them.
[02:19] <wgrant> Makes it very difficult to change anything.
[02:20] <cprov> right, you think I should implement also the removal on CRICSet methods, or should I return ResultSet to be removed on the callsite ?
[02:21] <wgrant> I think you should just implement a remove method. We don't expose any ResultSets from CRICS at the moment, and I can't really see any valid reason for someone to want one.
[02:23] <cprov> wgrant: agreed, also the removal() should really consider both CRIC and CRICD as it is implemented in BMP, right ? they are likely to be removed together every time.
[02:25] <wgrant> cprov: Right, that might well be a good thing to do.
[02:25] <wgrant> I can't see much of a reason to split them.
[02:32] <cprov> wgrant: code moved and pushed
[02:39] <wgrant> cprov: I'd rename the method to removeFromDiffs or similar, but that looks great otherwise. Thanks.
[02:41] <cprov> wgrant: no problemo, renamed version pushed
[02:42] <wgrant> cprov: How much longer are you likely to be around? I think I might implement a quick fix to only include commented files (not just hunks, at least not yet) in emails.
[02:42] <cprov> longer enough.
[04:36] <cprov> wgrant: http://lpbuildbot.canonical.com/builders/lucid_lp_lxc/builds/1172/steps/shell_9/logs/summary is that old spurious failure, right ?
[04:36] <wgrant> cprov: Yup, already forced.
[04:36] <cprov> wgrant: thanks
[04:37] <cprov> wgrant: how is the ic-email improvement ?
[08:20] <cjwatson> wgrant: Any luck with the livefs reviews?  I know there's been the whole pg9.3 thing, and lots of work on IC ...
[08:20] <cjwatson> I've put the remaining things people are asking for on the shelf for now, since I reached the point where I could do a touch image build with an added PPA
[08:22] <wgrant> cjwatson: Sorry, busy with staging and postgres and inline comments and PPA microservice and ephemeral PPAs and Malta... Three of those are coming to a close tomorrow, so I might actually have some time.
[08:24] <cjwatson> OK, thanks
[08:26] <wgrant> cjwatson: Is Laney's cron.germinate change sane? I saw some debate over what the name should be.
[08:57] <cjwatson> Oh, that bit of the naming is fine.  I was meaning to review and land that for him today.
[08:58] <cjwatson> It will cause tasks to appear for sdk-libs or something like that, but they were actually wondering why they weren't there anyway
[08:58] <cjwatson> And it will cause cron.germinate to take that bit longer, of course, but I've been looking at parallelising it ...
[09:14] <cjwatson> Laney: Could you please set a commit message for https://code.launchpad.net/~laney/launchpad/germinate-ubuntu-touch/+merge/220233 ?
[09:14] <Laney> cjwatson: Oh right, yes
[09:15] <Laney> try that
[09:17] <cjwatson> Laney: Thanks, landing
[09:17] <Laney> cheers
[12:05] <cjohnston> wgrant: have the new IC changes been released?
[12:05] <cjohnston> I guess maybe it's DC changes now? ;-)
[12:06] <wgrant> cjohnston: In progress.
[12:06] <cjohnston> cool
[12:06] <cjohnston> How'd the DB stuff go?
[12:06] <wgrant> Not quite uneventful, but eventually successful.
[12:06] <cjohnston> cool
[12:06] <wgrant> The master is now 9.3, and the slaves are rebuilding on 9.3.
[12:07] <cjohnston> oh nice.. so the upgrade was done..
[12:10] <wgrant> Yep
[12:10] <wgrant> I poked the comments stuff pretty thoroughly this evening, and fixed a few bugs
[12:11] <wgrant> Including a hilarious off-by-one error in the email formatter, where comments were showing up a line later, and the first line of the diff was always missing.
[12:16] <jelmer> wgrant: is launchpad really going to do inline comments on code reviews?
[12:17] <wgrant> jelmer: On Monday, with a bit of luck.
[12:17] <jelmer> wgrant: good stuff
[12:20] <cjohnston> by "with a bit of luck" he means pencil and paper
[12:21] <cjohnston> wgrant: take a look at https://code.launchpad.net/~ursinha/uci-engine/ts-add-mp-support/+merge/220275  I think we may need to rethink the design of my "comment" a little... cjohnston a moment ago: (and then nothing)
[12:21] <cjohnston> the way it is it looks really odd without any sort of review or comments or anything
[12:21] <wgrant> cjohnston: Yeah, I think we probably want to move the "Show diff comments" link into the body somewhere. We really need space on the bottom of each comment to have links
[12:22] <wgrant> Show diff comments, Reply, and Save/Cancel in the case of the inline editor.
[12:22] <wgrant> There's no way to have actions today.
[12:22] <wgrant> Also the Delete button, and possibly a future Edit feature
[12:22] <cjohnston> edit!
[12:22] <wgrant> It's all really awful atm, no consistency. And the inconsistent bits are dreadful anyway.
[12:58] <wgrant> cjohnston: prod's all up to date now, if you want to prepare that email.
[12:58] <cjohnston> ack. thanks
[23:02] <cjohnston> wgrant: cprov, what do you think about moving the 'Add comment' stuff to below the diff...
[23:02] <cjohnston> I think it would improve workflow
[23:02] <cjohnston> If you are replying to a comment you may not need to see the diff, but otherwise, your probably going to be adding a comment after reading the diff
[23:02] <cjohnston> so instead of having to click a link to go back up, why not just have the box below
[23:46] <wgrant> cjohnston: The problem is that the diff is enormous.
[23:46] <wgrant> This is why other sites have it on a separate tab.
[23:47] <cjohnston> it apparently isn't completely discoverable That you have to click save comment
[23:49] <wgrant> Yeah, that's one of the big remaining issues.
[23:49] <wgrant> But moving the comment form below the diff would make it very difficult to find.
[23:49] <wgrant> Even for MP-wide comments.
[23:50] <cjohnston> personally i would expect it to be at the bottom and not Where it is
[23:50] <cjohnston> it seems odd to have to scroll back up after scrolling to the bottom
[23:51] <wgrant> Sure, but then you have to scroll through 5000 lines of diff.
[23:51] <wgrant> Just to find the comment form
[23:51] <wgrant> + it separates the comment form from the comment thread
[23:51] <wgrant> Which is weird
[23:51] <cjohnston> or scroll 10k lines to read the diff and then go back up
[23:53] <cjohnston> what do you think about changing the new bottom link to be either 'return to add comment' or 'return to save comments' depending on if drafts are present or not
[23:54] <cjohnston> that may help discoverability slightly