| wgrant | cprov, cjohnston: people.canonical.com/~wgrant/launchpad/almost-final‽.png | 01:21 |
|---|---|---|
| cprov | wgrant: hey hey | 01:22 |
| 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:24 |
| 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:25 |
| cjohnston | looks good wgrant | 01:27 |
| wgrant | Someone want to review? https://code.launchpad.net/~wgrant/launchpad/ic-js-cleanup/+merge/219531 | 01:27 |
| cprov | wgrant: I will | 01:28 |
| cprov | wgrant: can you review https://code.launchpad.net/~cprov/launchpad/delete-branches-with-ics ? | 01:29 |
| 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:30 |
| 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:31 |
| 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:32 |
| cjohnston | I would argue that 'regular' comments are diff comments too.. | 01:33 |
| cjohnston | IC is actually inline | 01:33 |
| cprov | wgrant: table-layout: fixed, not only predictable but also faster/easier for browsers. Nice one! | 01:34 |
| cprov | had to look it up ... | 01:35 |
| wgrant | It's pretty useful here, since divs expand to fill their container, and tds expand to fit their contents. | 01:36 |
| 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:38 |
| cprov | ah, cool | 01:39 |
| wgrant | cprov: The rename is pushed. | 01:45 |
| cprov | wgrant: what does the CSS added in the BMP template do ? | 01:54 |
| 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:55 |
| cprov | I think I had fixed that in 17011, on top of cjohnston fixes ... | 01:57 |
| wgrant | Oh, so you did | 01:57 |
| wgrant | I'll uncommit and merge | 01:58 |
| 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:02 |
| wgrant | Also, you added a tab! | 02:03 |
| wgrant | Die | 02:03 |
| cprov | I must, I don't know why the emacs in my vm is allowing this shit ... I will review my config | 02:08 |
| 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:09 |
| wgrant | Heh | 02:10 |
| wgrant | cprov: The line-no col is wide enough to contain five digits plus the add sprite. | 02:15 |
| 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:17 |
| 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:18 |
| wgrant | Makes it very difficult to change anything. | 02:19 |
| 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:20 |
| 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:21 |
| 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:23 |
| 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:25 |
| cprov | wgrant: code moved and pushed | 02:32 |
| wgrant | cprov: I'd rename the method to removeFromDiffs or similar, but that looks great otherwise. Thanks. | 02:39 |
| cprov | wgrant: no problemo, renamed version pushed | 02:41 |
| 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. | 02:42 |
| 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:36 |
| cprov | wgrant: how is the ic-email improvement ? | 04:37 |
| 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:20 |
| 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:22 |
| cjwatson | OK, thanks | 08:24 |
| wgrant | cjwatson: Is Laney's cron.germinate change sane? I saw some debate over what the name should be. | 08:26 |
| cjwatson | Oh, that bit of the naming is fine. I was meaning to review and land that for him today. | 08:57 |
| 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 ... | 08:58 |
| 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:14 |
| Laney | try that | 09:15 |
| cjwatson | Laney: Thanks, landing | 09:17 |
| Laney | cheers | 09:17 |
| cjohnston | wgrant: have the new IC changes been released? | 12:05 |
| cjohnston | I guess maybe it's DC changes now? ;-) | 12:05 |
| 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:06 |
| cjohnston | oh nice.. so the upgrade was done.. | 12:07 |
| wgrant | Yep | 12:10 |
| wgrant | I poked the comments stuff pretty thoroughly this evening, and fixed a few bugs | 12:10 |
| 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:11 |
| jelmer | wgrant: is launchpad really going to do inline comments on code reviews? | 12:16 |
| wgrant | jelmer: On Monday, with a bit of luck. | 12:17 |
| jelmer | wgrant: good stuff | 12:17 |
| cjohnston | by "with a bit of luck" he means pencil and paper | 12:20 |
| 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:21 |
| 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:22 |
| wgrant | cjohnston: prod's all up to date now, if you want to prepare that email. | 12:58 |
| cjohnston | ack. thanks | 12:58 |
| === 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_ | ||
| 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:02 |
| 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:46 |
| cjohnston | it apparently isn't completely discoverable That you have to click save comment | 23:47 |
| 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:49 |
| 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:50 |
| 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:51 |
| 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:53 |
| cjohnston | that may help discoverability slightly | 23:54 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!