=== mwhudson_ is now known as mwhudson | ||
=== mup_ is now known as mup | ||
noodles775 | Topic for #launchpad-reviews: on call: noodles775 || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | 09:05 |
---|---|---|
al-maisan | jtv: good morning, did you see the reply to your review email by any chance? | 10:34 |
=== matsubara-afk is now known as matsubara | ||
jtv | al-maisan: going over there now | 11:27 |
al-maisan | jtv: thanks! | 11:28 |
jtv | al-maisan: shame there's always so little time at fosdem... we should've gotten a place with internet and stay for a few extra days of buildfarm work. :) | 11:30 |
al-maisan | jtv: at least a few days :) | 11:30 |
jtv | :) | 11:30 |
jtv | al-maisan, you are cleared to land | 11:32 |
al-maisan | jtv: oh, great, thanks! | 11:34 |
jtv | jml: could you have a look at this db change? I'm going to ask for the operative statement to be run on production as well. | 11:41 |
jml | no. | 11:41 |
jml | sorry, not for another couple of hours. | 11:41 |
jtv | jml: I'll shop around a bit then... it's pretty minimal, and I don't see stub around | 11:42 |
jtv | BjornT: is this something you can review? A "create index" to deal with timeouts on the Bugs front page. https://code.edge.launchpad.net/~jtv/launchpad/db-bug-518965/+merge/19091 | 11:52 |
deryck | jtv, I saw your bug about the need for the index, but I thought we removed the secondary sort on id. I'm curious why this is still needed. | 12:01 |
jtv | deryck: that's just what I'm trying to figure out now. The page loads in 9 seconds for me, so too short for a helpful oops but too long for human consumption. | 12:02 |
jtv | deryck: I thought gmb said that these were distinct problems, but when I actually looked I found I was mistaken. There may be other queries with the exact same problem. | 12:03 |
jtv | hmm... closer to 19s this time. Pretty bad. | 12:04 |
deryck | jtv, we were having other issues yesterday, which we don't normally have, we assume because we're only hitting the master dbs currently. this could be related, perhaps? | 12:06 |
jtv | deryck: is that still the case? Yes, that'd increase load by a lot. | 12:06 |
deryck | jtv, yes, that is the case. | 12:06 |
deryck | jtv, gary was reluctant to land the change to re-enable slaves with stub unavailable. | 12:07 |
jtv | deryck: we used to ratchet down the timeout for performance weeks (no idea why we stopped doing that), and this may have the same effect. | 12:07 |
deryck | jtv, yeah, makes sense to me. | 12:09 |
jtv | deryck: I'm now checking out edge oopses to see whether the problem is really solved. | 12:10 |
jtv | deryck: the answer is somewhere in the middle—this particular bottleneck has been resolved (sorry for mixing metaphors) but other queries are still slow. | 12:17 |
jtv | I'm withdrawing my patch. | 12:17 |
deryck | jtv, ok, cool. | 12:27 |
=== mrevell is now known as mrevell-lunch | ||
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
=== mrevell-lunch is now known as mrevell | ||
jtv | noodles775: did you say intellectronica was going to do my UI review for https://code.edge.launchpad.net/~jtv/launchpad/translationimportqueueentry-info/+merge/18963 ? | 13:51 |
intellectronica | jtv: i'm not sure what the current ui review policy is, but unless it changed, i can only do a mentored review (which i'll be happy to do) | 13:52 |
noodles775 | jtv: nope, I said that it'd be good to check with sinzui as afaik he's wanting to build up ui reviews. | 13:52 |
jtv | noodles775: ah, so I remembered the wrong person | 13:52 |
noodles775 | If he doesn't want it, I can do it of course. | 13:52 |
jtv | intellectronica: sorry to have bothered you | 13:52 |
sinzui | I can do the review in an hour | 13:52 |
jtv | noodles775: I think it's a good mentoring review | 13:52 |
intellectronica | no worries | 13:52 |
jtv | sinzui: that'd be great, thanks | 13:52 |
sinzui | noodles775: has one ahead of your i think | 13:53 |
noodles775 | well I think jtv's is older than mine, so I'm happy to wait ;) | 13:56 |
jtv | mine's about a day and a half old | 13:57 |
mrevell | hey noodles775, wanna do another UI pre-imp chat? It's a very small matter. | 14:10 |
noodles775 | mrevell: sure, call or chat? | 14:16 |
mrevell | noodles775, Chat'll be fine | 14:16 |
noodles775 | k | 14:16 |
=== salgado is now known as salgado-lunch | ||
=== matsubara is now known as matsubara-lunch | ||
leonardr | rockstar, when you come in, hopefully the last lazr.restful multiversion branch for a while | 15:28 |
leonardr | https://code.edge.launchpad.net/~leonardr/lazr.restful/bump-version/+merge/19107 | 15:28 |
rockstar | leonardr, I'm actually composing the "holy crap I'm still sick" email. | 15:29 |
leonardr | rockstar: want me to give it to someone else? | 15:29 |
rockstar | leonardr, yes please. | 15:29 |
leonardr | noodles775, can you take https://code.edge.launchpad.net/~leonardr/lazr.restful/bump-version/+merge/19107 ? | 15:29 |
noodles775 | leonardr: I'm just on the phone, but I'll take a look as soon as I'm off. | 15:36 |
mrevell | Hello noodles775 | 15:47 |
mrevell | When you have a moment, I wanted to ask you for some UI advice. At UDS, deryck and I identified various bits of the bugs interface that could benefit from a help pop-up. | 15:48 |
noodles775 | yep | 15:48 |
mrevell | noodles775, One was the tags listing, which is also an ajax editing form. | 15:48 |
mrevell | I was thinking about adding a <small>(<a href="">?</a></small> just beside the pencil icon (or "Add tags" link, if there are no tags). It looks a little, well, spare. So I wondered about adding the helping to the "Tags" label. I'm not sure. Any thoughts? | 15:49 |
* noodles775 scans for other places where we've done something similar... | 15:50 | |
noodles775 | mrevell: so on the PPA index page, we have things like (in the context of uploading packages): "(Read about uploading)", but the page itself isn't as full as the bugs page... | 15:52 |
mrevell | noodles775, Yeah, and I think a full text link would detract from the tags themselves. | 15:52 |
=== deryck is now known as deryck[lunch] | ||
noodles775 | yep..., but that doesn't leave you many options? | 15:53 |
mrevell | Nope, hence this chat :) | 15:54 |
noodles775 | We do have: https://edge.launchpad.net/@@/maybe on the +graphics page. | 15:55 |
noodles775 | It looks much nicer than a text ?. | 15:55 |
mrevell | noodles775, Yeah, that's probably a good option. I prefer help links that include some context but not at the expense of cluttering up the page. I'll use that and see how it looks. Thanks. | 15:55 |
noodles775 | np. | 15:56 |
=== matsubara-lunch is now known as matsubara | ||
noodles775 | leonardr: on line 45 of the MP diff, is this really the earliest version, or the earliest *active* version? (which line 47 seems to imply?) | 16:02 |
leonardr | noodles775: inactive versions don't exist in the lazr.restful installation, only in peoples' memories, so the concepts are equivalent | 16:03 |
leonardr | i can change the wording | 16:03 |
noodles775 | leonardr: in which case, why can len(config.active_versions) == 0, but we still have an earliest_version? or is that what you'll re-word? | 16:03 |
leonardr | noodles775: it's kind of confusing but i don't want to change the interface becuase that'll cause backward incompatibility problems | 16:04 |
leonardr | there are two fields in the interface | 16:04 |
leonardr | active_versions and latest_version_uri_prefix | 16:04 |
leonardr | active_versions is a list, the other is a stirng | 16:04 |
leonardr | lazr.restful serves web services for all of those strings | 16:05 |
leonardr | so if active_versions is =['beta', '1.0'] and latest_version_uri_prefix is 'devel' there are three versions being served | 16:05 |
leonardr | if active_versions is [] and latest_version_uri_prefix is 'beta' then there is one version | 16:05 |
leonardr | though i don't recommend doing that | 16:05 |
noodles775 | So if active_versions is =['beta', '1.0'] and latest_version_uri_prefix is 'devel', why are we evaluating earliest_version = 'beta', is that right? | 16:06 |
noodles775 | leonardr: ^^ | 16:06 |
leonardr | noodles775: i'm having trouble parsing that sentence, but i can tell you that in that case earliest_version is 'beta' | 16:07 |
leonardr | latest_version_uri_prefix can only be the earliest version if active_versions is empty | 16:07 |
noodles775 | OK, as the code implies, I'm just confused by the names. | 16:07 |
leonardr | noodles775: i'm kind of leaning towards treating the last item in active_versions as latest_version_uri_prefix | 16:08 |
leonardr | but that would be a separate job | 16:08 |
=== salgado-lunch is now known as salgado | ||
noodles775 | Right, that would make sense. (do you mind adding an XXX?) | 16:09 |
leonardr | sure | 16:09 |
noodles775 | leonardr: just a style question, is the comment on the empty line 69 intentional? | 16:11 |
leonardr | yeah, it's like a para break. i don't know if we allow that though | 16:11 |
noodles775 | Yeah, I think a blank line (without the comment) serves just as well, but if the style guide doesn't say either way, whatever you prefer :) | 16:12 |
leonardr | style guide doesn't mention it | 16:12 |
leonardr | if i see a blank line i think the first comment was talking about the blank line and the second comment is different | 16:13 |
noodles775 | leonardr: s/marker_interface/version_marker on line 72 | 16:13 |
noodles775 | sure | 16:13 |
leonardr | noodles775: i might as well make that latest_version_uri_prefix change because i haven't actually released a version that uses it yet. if i get rid of it now there will be no backwards compatibility | 16:20 |
leonardr | problem | 16:20 |
noodles775 | leonardr: great | 16:20 |
noodles775 | Two other small things: | 16:20 |
noodles775 | See if you think it's worth creating a helper for the repeats of line 145-146 - the helper could even print out the ordered list items? | 16:21 |
noodles775 | s/repeats/repititions sorry, bad day. | 16:22 |
noodles775 | And s/an/a on line 180. | 16:22 |
leonardr | noodles775: i thought about putting that 145-146 stuff in the big helper method but decided it was easier to read if i spelled it out every time | 16:26 |
noodles775 | leonardr: OK. | 16:27 |
noodles775 | leonardr: another typo, s/in/In on 138 | 16:28 |
leonardr | noodles775, fixed everything so far | 16:29 |
noodles775 | leonardr: Great, I think the text for versions 3.0 is incorrect? The method has changed to by_value? | 16:31 |
noodles775 | leonardr: and with that, r=me. | 16:31 |
leonardr | yes, you're right | 16:31 |
leonardr | noodles775: if you can move on to https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/multiversion/+merge/19110 i'll work on getting rid of latest_version_uri_prefix | 16:31 |
noodles775 | leonardr: sorry, I need to leave asap now, but I've got one more question for the this MP: | 16:32 |
leonardr | ok | 16:32 |
noodles775 | Is the change in tales.py tested? | 16:32 |
noodles775 | I can't see an example where we we've removed an op in a later version? | 16:33 |
leonardr | noodles775: i don't have details off the top of my head, but it is tested in that the wadl.txt test will fail if you remove the code | 16:34 |
leonardr | noodles775: actually i can add a quick test that proves it works | 16:34 |
noodles775 | OK, great. I'm out of here... 'night! | 16:35 |
leonardr | can i get someone else interested in the web service to review some more branches? maybe sinzui? | 16:37 |
noodles775 | on call: - || reviewing: - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | 16:38 |
=== noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
sinzui | leonardr: I have reviews ahead of you, but I can commit to doing it in a few hours | 16:39 |
leonardr | sinzui: ok, let me try to find someone else. EdwinGrubbs? gary? intellectronica? | 16:40 |
intellectronica | leonardr: you're asking for a review? | 16:40 |
leonardr | intellectronica: yes, if oyu have the time | 16:40 |
intellectronica | how much work is it? | 16:40 |
leonardr | not much if you only want to do one | 16:40 |
leonardr | i have a couple, but i'm taking reviewers as i can find them | 16:40 |
intellectronica | oright, i'll do one then | 16:40 |
leonardr | intellectronica: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/multiversion/+merge/19110 | 16:41 |
intellectronica | leonardr: the diff looks fine to me | 16:42 |
leonardr | cool | 16:43 |
gary_poster | leonardr: I'll have some time late afternoon | 16:43 |
intellectronica | leonardr: i wonder if it would be good to extract the code that deals with building the URL to another method, so that a specialized class can override it? | 16:43 |
intellectronica | i mean if an implementation needs to build the URL using a rule other than base + '/' + version | 16:44 |
leonardr | intellectronica: i think yagni. if you want that you can subclass the client class and put your implementation in the constructor before calling the superconstructor | 16:44 |
intellectronica | fair enough. r=me | 16:44 |
=== deryck[lunch] is now known as deryck | ||
leonardr | flacoste, maybe you want to review the branch i chatted with you about earlier? | 18:20 |
leonardr | shouldn't take long | 18:20 |
leonardr | https://code.edge.launchpad.net/~leonardr/lazr.restful/no-latest-version/+merge/19122 | 18:20 |
leonardr | or gary or sinzui, if you're free by now | 18:28 |
gary_poster | leonardr, I'll take it | 18:29 |
leonardr | gary: link above | 18:29 |
gary_poster | ack, looking | 18:30 |
gary_poster | leonardr: nice. approved. | 18:33 |
leonardr | gary, thanks | 18:33 |
jtv | sinzui: do you think you'll get around to that UI review? | 19:20 |
jtv | there's a link to some screenshots in my last comment | 19:20 |
sinzui | jtv: I am just finishing the one before you | 19:20 |
jtv | cool | 19:20 |
sinzui | jtv: ping | 19:53 |
jtv | sinzui: hi | 19:53 |
jtv | sinzui: there are some rough edges, I know; I'm at your disposal | 19:53 |
sinzui | jtv: your edit links (imports/2) do not work in webkit. | 19:53 |
sinzui | I can see the edit icons in firefox, but not epiphany | 19:53 |
jtv | sinzui: that's a known bug; I already submitted a fix to lazr-js | 19:54 |
sinzui | fab. I can do the review now that I know where the link to your changes are | 19:54 |
jtv | (also, not a big problem here given how few people use this form) | 19:54 |
jtv | sinzui: one of those rough edges... I put the extra information at the bottom of the page, below the form buttons. I'm not sure if that's acceptable or not. | 19:55 |
sinzui | There are examples like that already | 19:56 |
jtv | phew :) | 19:57 |
jtv | the other thing is that the first <li> of the <ul> is getting a little too cozy with the line above the <ul> | 19:57 |
sinzui | jtv: but as you know, most forms would put that information about the form. How is this information secondary, or distractive that implies it should be below? | 19:57 |
sinzui | in webkit there is 300px between the form field and the submit buttons. I do not see where the markup error is yet | 19:58 |
jtv | sinzui: I didn't want to put them at the top because they would force scrolling on an admin who goes through the current workflow; and I didn't put them on the side because this is a generic form with a lot of JS to hide and show sections, and I'm not sure how safe it would be. | 19:59 |
jtv | sinzui: I'm not too worried about webkit, since we've really only got about 4 people who use this form regularly and perhaps the same number again who can access it. | 20:00 |
sinzui | jtv: the error usually indicates that there is a deep error the firefox is not showing you yet. | 20:01 |
sinzui | jtv: for example: mailing list moderate was broken for webkit, so it was ignored for 4 months, when I fix it, other submit errors in firfox were also fixed | 20:02 |
jtv | sinzui: in this case it may be something to do with the existing JS machinery in the form... nobody would have noticed that, I think | 20:02 |
* jtv tries | 20:03 | |
jtv | sinzui: definitely not 300 pixels for the form on staging... I'll try to reproduce on dev. Meanwhile, try selecting a different filetype and watching the JS reshape the form. It may tell you something. | 20:06 |
jtv | sinzui: oh! On staging, in konqueror, I get those 300 px or so, right in the middle of the form. | 20:07 |
jtv | Depending on what I do with that "file type" dropdown | 20:08 |
jtv | sinzui: so that's a pre-existing bug, very low-priority because of the limited usage of this page. | 20:08 |
sinzui | right. The: markup is well formed, but the error implies the table is ill. | 20:08 |
jtv | It might just have something to do with the sluggy response I've been getting from firefox on this form. | 20:09 |
jtv | *sloggy | 20:09 |
sinzui | jtv: your changes are good too land. I have a minor dislike to the whitespace between the two columns you added. I do not see it normally because I keep my browser narrow. So long as you admins don't mind the large gutter of white-space, land away | 20:10 |
jtv | sinzui: yay! thanks | 20:10 |
jtv | sinzui: no mentor approval needed? | 20:11 |
jtv | "sluggish," _that_ was the word | 20:12 |
sinzui | Anther UI is needed, but not necessarily noodles775. We are using the buddy system where two crazed developers can approve the UI is a suicide pact. | 20:13 |
=== matsubara is now known as matsubara-afk | ||
=== salgado is now known as salgado-afk | ||
bac | hi sinzui: https://code.launchpad.net/~bac/launchpad/bug-490521/+merge/19133 | 20:16 |
sinzui | jtv; the broken edit icon is insane markup, not javascript | 20:23 |
jtv | sinzui: what went wrong? | 20:23 |
sinzui | The anchor is around a span that uses class="invisible-link". The CSS instruction means no not render. firefox is ignoring the instruction. The css does mean do not render because we use it to insert data for scraping, not seeing | 20:25 |
jtv | sinzui: wow, no idea how that got there... unless it was as a way to hide the icon from non-admins. | 20:26 |
sinzui | We created it for diagnostic testing of link rendering | 20:27 |
jtv | sinzui: hmm... afaics only the span *inside* the link is invisible-link. There should still be an edit sprite, and I think that is the lazr-js bug | 20:28 |
sinzui | jtv: the issue is a little more complex that that. I will find the bug in rosetta and update it with the correct fix | 20:28 |
sinzui | jtv: I do not see any javascript | 20:29 |
sinzui | jtv: is something rewriting the link? | 20:29 |
* sinzui see the template and markup look like simple html | 20:30 | |
jtv | sinzui: no, it's done in TAL after all... there's a doctest using the invisible link though. | 20:31 |
sinzui | jtv: oh, the fix is trivial, it is the white-space inside the anchor that is wrong | 20:31 |
sinzui | intellectronica: had me fix a similar issue two weeks ago | 20:32 |
sinzui | jtv: the <span> must abut the <a>: | 20:32 |
sinzui | <tal:block condition="entry/required:launchpad.Admin"> | 20:32 |
sinzui | <a class="sprite edit" | 20:32 |
sinzui | tal:attributes="href entry/fmt:url"><span class="invisible-link">Change | 20:32 |
sinzui | this entry</span></a> | 20:32 |
sinzui | </tal:block> | 20:32 |
jtv | sinzui: DON'T tell me the CSS does something with :first here... | 20:34 |
sinzui | I am unsure what the issue bug the fix is trivial: | 20:36 |
jtv | no whitespace between the <a> and the <span>. Gotcha. | 20:37 |
sinzui | dude! | 20:37 |
sinzui | span.invisible {left:-9999em;display:block;} | 20:37 |
jtv | ? | 20:38 |
sinzui | The link was pushed passed the phone on my desk | 20:38 |
sinzui | sorry. looking at the wrong entry: span.invisible-link {display:none;} is as I would expect | 20:39 |
* jtv was wondering since when css classes had inheritance :-) | 20:39 | |
jtv | sinzui: I'm filing a bug for this... thanks, because I can confidently say I would not have figured this out | 20:40 |
sinzui | bac: I think your template is missing the display of translations | 20:51 |
jtv | sinzui: was that meant for me? | 20:51 |
bac | jtv: i think it was for me | 20:53 |
sinzui | jtv: no. bac is working on a branch that shows what upstream information is missing for a source package : http://people.canonical.com/~bac/upstream-portlet.png | 20:53 |
bac | jtv: at least my bell went off | 20:53 |
jtv | ah ok, just checking because of the T word and proximity to conversation with YT. :) | 20:53 |
sinzui | If the source package has translation templates, then we need to check if auto imports were enabled for the series branch | 20:54 |
sinzui | jtv: I pondered the confusion as I was typing it | 20:54 |
bac | sinzui: ok. | 20:55 |
sinzui | bac: this might work: http://pastebin.ubuntu.com/374223/ | 20:59 |
jtv | sinzui: I don't suppose you'd also like to review this one? | 21:04 |
sinzui | your branch from a few weeks ago? | 21:04 |
jtv | sinzui: no, this fix you just gave me | 21:06 |
jtv | sinzui: mainly interesting for the before/after pictures: https://code.launchpad.net/~jtv/launchpad/bug-520659/+merge/19140 | 21:06 |
sinzui | jtv r=me | 21:07 |
jtv | sinzui: thanks :) | 21:07 |
sinzui | jtv: I think we can ignore konquerer issues when Lucid is released because the default will be webkit-based | 21:07 |
jtv | sinzui: for this, that means we can start ignoring konqueror issues when lucid is in late beta. :-) | 21:08 |
sinzui | jtv: there are about 20 such bugs we can mark as wont fix | 21:09 |
jtv | sinzui: so the support for konqueror is basically, "whatever is current in ubuntu"? | 21:12 |
sinzui | I think we need to take traffic in consideration...IE6 still haunts US. konqueror is small ans appears to follow the latest release | 21:13 |
sinzui | jtv: Chrome is webkit and Epiphany switched to webkit. so we are seeing a large increase in webkit; we once felt pretty comfortable saying we will not fix safari bugs quickly. | 21:15 |
jtv | sinzui: I'm surprised about epiphany | 21:15 |
sinzui | not me, gecko was always a pain to build and keep up with. gecok was created for the mozilla stack, not for embedding | 21:16 |
jtv | I suppose a mature but "fresh" engine without too much historical baggage has its attractions... | 21:18 |
jtv | as ddaa remarked the other day, things would be very different if xhtml had arrived in a time like this when browsers are actually evolving | 21:18 |
* jtv -> afk | 21:19 | |
=== jtv is now known as jtv-afk | ||
bac | sinzui: more better? http://people.canonical.com/~bac/upstream-portlet.png | 21:22 |
sinzui | wow, yes | 21:23 |
sinzui | bac: you lost the edit link, and in reflection, I think that indicates that it was not very clear... | 21:24 |
* bac goes back to look at devel | 21:25 | |
bac | sinzui: so you're talking about the edit link on the series, correct? | 21:27 |
sinzui | bac: I think we should move the link to edit the Packaging link to the horizontal list where we can use a clear label. Maybe "(/) Change upstream link" | 21:27 |
sinzui | bac: yes, that was editing the link, but it was not clear that you could change the project and series. | 21:28 |
sinzui | oh, and I lost the icon in a past revision. I suck | 21:29 |
bac | sinzui: so change "Show upstream links" to "Change upstream link" and put it to the right of "View changelog"? | 21:31 |
sinzui | umm, no | 21:31 |
bac | yeah, you've lost me then | 21:31 |
sinzui | The link in the horizontal list is goes to the right place, but it is missing an icon (i) -- surely that is my mistake | 21:32 |
sinzui | bac: Instead of restoring the link you lost in the diff, we should move it to this same horizontal ul and use "Change upstream link" to make it very clear that the user can change the project and series he knows it is wrong | 21:34 |
bac | sinzui: ring ring | 21:34 |
bac | the mic works | 21:35 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!