[01:21] cprov, cjohnston: people.canonical.com/~wgrant/launchpad/almost-final‽.png [01:22] wgrant: hey hey [01:24] it is coming up very nicely and since the new layout is more flexible (div) it will be easier to increment. [01:24] I hope so. [01:25] wgrant: I've just opened an asana task about finding a way to materialise the IC threads, specially in emails [01:25] 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] Yeah, I've been wondering about how to do that. [01:25] It's a bit awkward, as they're all at different depths potentially. [01:25] wgrant: it's annoying to to read the reviews right now [01:27] looks good wgrant [01:27] Someone want to review? https://code.launchpad.net/~wgrant/launchpad/ic-js-cleanup/+merge/219531 [01:28] wgrant: I will [01:29] wgrant: can you review https://code.launchpad.net/~cprov/launchpad/delete-branches-with-ics ? [01:30] Hm [01:30] What do you guys think about renaming "inline" to "diff"? [01:30] That pretty much just affects the "Include X inline comments" checkbox [01:31] Makes it clearer what it refers to. [01:31] wgrant: I wonder if I can just add few other 'ouch' in your MP and let you figure them out ;-) [01:31] Heh [01:31] +1, more ouch [01:31] wgrant: +1 on "diff comments" [01:32] if you do that you should probably do the same in the email [01:32] what about 'View inline comments' [01:32] Oh yeah, email and link too, forgot those. [01:33] I would argue that 'regular' comments are diff comments too.. [01:33] IC is actually inline [01:34] wgrant: table-layout: fixed, not only predictable but also faster/easier for browsers. Nice one! [01:35] had to look it up ... [01:36] It's pretty useful here, since divs expand to fill their container, and tds expand to fit their contents. [01:38] wgrant: what about the TextWidget for review_type ? what does it bring ? [01:38] cprov: I narrowed it from 20 characters to 15, to ensure that the widgets all fit on one line. [01:39] ah, cool [01:45] cprov: The rename is pushed. [01:54] wgrant: what does the CSS added in the BMP template do ? [01:55] cprov: That's a variation on cjohnston's. It adds a bit of space before the publish checkbox. [01:55] 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] I think I had fixed that in 17011, on top of cjohnston fixes ... [01:57] Oh, so you did [01:58] I'll uncommit and merge [02:02] cprov: Have you seen any more issues on (qa)staging since those firewall fixes a few hours ago? [02:02] 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] Also, you added a tab! [02:03] Die [02:08] I must, I don't know why the emacs in my vm is allowing this shit ... I will review my config [02:09] 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] Heh [02:15] cprov: The line-no col is wide enough to contain five digits plus the add sprite. [02:17] up to 99999 lines diffs then, not at problem [02:17] Well, if you go back to your old days... :P [02:17] wgrant: so, you really want new methods on CRICSet ... [02:18] cprov: All the other manipulation code (apart from person merge) is in codereviewinlinecomment.py [02:18] 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] Makes it very difficult to change anything. [02:20] 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] 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] 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] cprov: Right, that might well be a good thing to do. [02:25] I can't see much of a reason to split them. [02:32] wgrant: code moved and pushed [02:39] cprov: I'd rename the method to removeFromDiffs or similar, but that looks great otherwise. Thanks. [02:41] wgrant: no problemo, renamed version pushed [02:42] 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] longer enough. [04:36] wgrant: http://lpbuildbot.canonical.com/builders/lucid_lp_lxc/builds/1172/steps/shell_9/logs/summary is that old spurious failure, right ? [04:36] cprov: Yup, already forced. [04:36] wgrant: thanks [04:37] wgrant: how is the ic-email improvement ? [08:20] 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] 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] 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] OK, thanks [08:26] cjwatson: Is Laney's cron.germinate change sane? I saw some debate over what the name should be. [08:57] Oh, that bit of the naming is fine. I was meaning to review and land that for him today. [08:58] 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] And it will cause cron.germinate to take that bit longer, of course, but I've been looking at parallelising it ... [09:14] Laney: Could you please set a commit message for https://code.launchpad.net/~laney/launchpad/germinate-ubuntu-touch/+merge/220233 ? [09:14] cjwatson: Oh right, yes [09:15] try that [09:17] Laney: Thanks, landing [09:17] cheers [12:05] wgrant: have the new IC changes been released? [12:05] I guess maybe it's DC changes now? ;-) [12:06] cjohnston: In progress. [12:06] cool [12:06] How'd the DB stuff go? [12:06] Not quite uneventful, but eventually successful. [12:06] cool [12:06] The master is now 9.3, and the slaves are rebuilding on 9.3. [12:07] oh nice.. so the upgrade was done.. [12:10] Yep [12:10] I poked the comments stuff pretty thoroughly this evening, and fixed a few bugs [12:11] 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] wgrant: is launchpad really going to do inline comments on code reviews? [12:17] jelmer: On Monday, with a bit of luck. [12:17] wgrant: good stuff [12:20] by "with a bit of luck" he means pencil and paper [12:21] 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] the way it is it looks really odd without any sort of review or comments or anything [12:21] 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] Show diff comments, Reply, and Save/Cancel in the case of the inline editor. [12:22] There's no way to have actions today. [12:22] Also the Delete button, and possibly a future Edit feature [12:22] edit! [12:22] It's all really awful atm, no consistency. And the inconsistent bits are dreadful anyway. [12:58] cjohnston: prod's all up to date now, if you want to prepare that email. [12:58] ack. thanks === Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === cprov_ is now known as cprov === _mup__ is now known as _mup_ [23:02] wgrant: cprov, what do you think about moving the 'Add comment' stuff to below the diff... [23:02] I think it would improve workflow [23:02] 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] so instead of having to click a link to go back up, why not just have the box below [23:46] cjohnston: The problem is that the diff is enormous. [23:46] This is why other sites have it on a separate tab. [23:47] it apparently isn't completely discoverable That you have to click save comment [23:49] Yeah, that's one of the big remaining issues. [23:49] But moving the comment form below the diff would make it very difficult to find. [23:49] Even for MP-wide comments. [23:50] personally i would expect it to be at the bottom and not Where it is [23:50] it seems odd to have to scroll back up after scrolling to the bottom [23:51] Sure, but then you have to scroll through 5000 lines of diff. [23:51] Just to find the comment form [23:51] + it separates the comment form from the comment thread [23:51] Which is weird [23:51] or scroll 10k lines to read the diff and then go back up [23:53] 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] that may help discoverability slightly