/srv/irclogs.ubuntu.com/2010/02/11/#launchpad-reviews.txt

=== mwhudson_ is now known as mwhudson
=== mup_ is now known as mup
noodles775Topic for #launchpad-reviews: on call: noodles775 || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews09:05
al-maisanjtv: good morning, did you see the reply to your review email by any chance?10:34
=== matsubara-afk is now known as matsubara
jtval-maisan: going over there now11:27
al-maisanjtv: thanks!11:28
jtval-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-maisanjtv: at least a few days :)11:30
jtv:)11:30
jtval-maisan, you are cleared to land11:32
al-maisanjtv: oh, great, thanks!11:34
jtvjml: 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
jmlno.11:41
jmlsorry, not for another couple of hours.11:41
jtvjml: I'll shop around a bit then... it's pretty minimal, and I don't see stub around11:42
jtvBjornT: 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/1909111:52
deryckjtv, 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
jtvderyck: 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
jtvderyck: 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
jtvhmm... closer to 19s this time.  Pretty bad.12:04
deryckjtv, 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
jtvderyck: is that still the case?  Yes, that'd increase load by a lot.12:06
deryckjtv, yes, that is the case.12:06
deryckjtv, gary was reluctant to land the change to re-enable slaves with stub unavailable.12:07
jtvderyck: 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
deryckjtv, yeah, makes sense to me.12:09
jtvderyck: I'm now checking out edge oopses to see whether the problem is really solved.12:10
jtvderyck: 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
jtvI'm withdrawing my patch.12:17
deryckjtv, 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
jtvnoodles775: 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
intellectronicajtv: 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
noodles775jtv: nope, I said that it'd be good to check with sinzui as afaik he's wanting to build up ui reviews.13:52
jtvnoodles775: ah, so I remembered the wrong person13:52
noodles775If he doesn't want it, I can do it of course.13:52
jtvintellectronica: sorry to have bothered you13:52
sinzuiI can do the review in an hour13:52
jtvnoodles775: I think it's a good mentoring review13:52
intellectronicano worries13:52
jtvsinzui: that'd be great, thanks13:52
sinzuinoodles775: has one ahead of your i think13:53
noodles775well I think jtv's is older than mine, so I'm happy to wait ;)13:56
jtvmine's about a day and a half old13:57
mrevellhey noodles775, wanna do another UI pre-imp chat? It's a very small matter.14:10
noodles775mrevell: sure, call or chat?14:16
mrevellnoodles775, Chat'll be fine14:16
noodles775k14:16
=== salgado is now known as salgado-lunch
=== matsubara is now known as matsubara-lunch
leonardrrockstar, when you come in, hopefully the last lazr.restful multiversion branch for a while15:28
leonardrhttps://code.edge.launchpad.net/~leonardr/lazr.restful/bump-version/+merge/1910715:28
rockstarleonardr, I'm actually composing the "holy crap I'm still sick" email.15:29
leonardrrockstar: want me to give it to someone else?15:29
rockstarleonardr, yes please.15:29
leonardrnoodles775, can you take https://code.edge.launchpad.net/~leonardr/lazr.restful/bump-version/+merge/19107 ?15:29
noodles775leonardr: I'm just on the phone, but I'll take a look as soon as I'm off.15:36
mrevellHello noodles77515:47
mrevellWhen 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
noodles775yep15:48
mrevellnoodles775, One was the tags listing, which is also an ajax editing form.15:48
mrevellI 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
noodles775mrevell: 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
mrevellnoodles775, Yeah, and I think a full text link would detract from the tags themselves.15:52
=== deryck is now known as deryck[lunch]
noodles775yep..., but that doesn't leave you many options?15:53
mrevellNope, hence this chat :)15:54
noodles775We do have: https://edge.launchpad.net/@@/maybe on the +graphics page.15:55
noodles775It looks much nicer than a text ?.15:55
mrevellnoodles775, 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
noodles775np.15:56
=== matsubara-lunch is now known as matsubara
noodles775leonardr: 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
leonardrnoodles775: inactive versions don't exist in the lazr.restful installation, only in peoples' memories, so the concepts are equivalent16:03
leonardri can change the wording16:03
noodles775leonardr: 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
leonardrnoodles775: it's kind of confusing but i don't want to change the interface becuase that'll cause backward incompatibility problems16:04
leonardrthere are two fields in the interface16:04
leonardractive_versions and latest_version_uri_prefix16:04
leonardractive_versions is a list, the other is a stirng16:04
leonardrlazr.restful serves web services for all of those strings16:05
leonardrso if active_versions is =['beta', '1.0'] and latest_version_uri_prefix is 'devel' there are three versions being served16:05
leonardrif active_versions is [] and latest_version_uri_prefix is 'beta' then there is one version16:05
leonardrthough i don't recommend doing that16:05
noodles775So 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
noodles775leonardr: ^^16:06
leonardrnoodles775: i'm having trouble parsing that sentence, but i can tell you that in that case earliest_version is 'beta'16:07
leonardrlatest_version_uri_prefix can only be the earliest version if active_versions is empty16:07
noodles775OK, as the code implies, I'm just confused by the names.16:07
leonardrnoodles775: i'm kind of leaning towards treating the last item in active_versions as latest_version_uri_prefix16:08
leonardrbut that would be a separate job16:08
=== salgado-lunch is now known as salgado
noodles775Right, that would make sense. (do you mind adding an XXX?)16:09
leonardrsure16:09
noodles775leonardr: just a style question, is the comment on the empty line 69 intentional?16:11
leonardryeah, it's like a para break. i don't know if we allow that though16:11
noodles775Yeah, 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
leonardrstyle guide doesn't mention it16:12
leonardrif i see a blank line i think the first comment was talking about the blank line and the second comment is different16:13
noodles775leonardr: s/marker_interface/version_marker on line 7216:13
noodles775sure16:13
leonardrnoodles775: 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 compatibility16:20
leonardrproblem16:20
noodles775leonardr: great16:20
noodles775Two other small things:16:20
noodles775See 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
noodles775s/repeats/repititions sorry, bad day.16:22
noodles775And s/an/a on line 180.16:22
leonardrnoodles775: 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 time16:26
noodles775leonardr: OK.16:27
noodles775leonardr: another typo, s/in/In on 13816:28
leonardrnoodles775, fixed everything so far16:29
noodles775leonardr: Great, I think the text for versions 3.0 is incorrect? The method has changed to by_value?16:31
noodles775leonardr: and with that, r=me.16:31
leonardryes, you're right16:31
leonardrnoodles775: 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_prefix16:31
noodles775leonardr: sorry, I need to leave asap now, but I've got one more question for the this MP:16:32
leonardrok16:32
noodles775Is the change in tales.py tested?16:32
noodles775I can't see an example where we we've removed an op in a later version?16:33
leonardrnoodles775: 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 code16:34
leonardrnoodles775: actually i can add a quick test that proves it works16:34
noodles775OK, great. I'm out of here... 'night!16:35
leonardrcan i get someone else interested in the web service to review some more branches? maybe sinzui?16:37
noodles775on call: - || reviewing: - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews16: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
sinzuileonardr: I have reviews ahead of you, but I can commit to doing it in a few hours16:39
leonardrsinzui: ok, let me try to find someone else. EdwinGrubbs? gary? intellectronica?16:40
intellectronicaleonardr: you're asking for a review?16:40
leonardrintellectronica: yes, if oyu have the time16:40
intellectronicahow much work is it?16:40
leonardrnot much if you only want to do one16:40
leonardri have a couple, but i'm taking reviewers as i can find them16:40
intellectronicaoright, i'll do one then16:40
leonardrintellectronica: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/multiversion/+merge/1911016:41
intellectronicaleonardr: the diff looks fine to me16:42
leonardrcool16:43
gary_posterleonardr: I'll have some time late afternoon16:43
intellectronicaleonardr: 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
intellectronicai mean if an implementation needs to build the URL using a rule other than base + '/' + version16:44
leonardrintellectronica: i think yagni. if you want that you can subclass the client class and put your implementation in the constructor before calling the superconstructor16:44
intellectronicafair enough. r=me16:44
=== deryck[lunch] is now known as deryck
leonardrflacoste, maybe you want to review the branch i chatted with you about earlier?18:20
leonardrshouldn't take long18:20
leonardrhttps://code.edge.launchpad.net/~leonardr/lazr.restful/no-latest-version/+merge/1912218:20
leonardror gary or sinzui, if you're free by now18:28
gary_posterleonardr, I'll take it18:29
leonardrgary: link above18:29
gary_posterack, looking18:30
gary_posterleonardr: nice.  approved.18:33
leonardrgary, thanks18:33
jtvsinzui: do you think you'll get around to that UI review?19:20
jtvthere's a link to some screenshots in my last comment19:20
sinzuijtv: I am just finishing the one before you19:20
jtvcool19:20
sinzuijtv: ping19:53
jtvsinzui: hi19:53
jtvsinzui: there are some rough edges, I know; I'm at your disposal19:53
sinzuijtv: your edit links (imports/2) do not work in webkit.19:53
sinzuiI can see the edit icons in firefox, but not epiphany19:53
jtvsinzui: that's a known bug; I already submitted a fix to lazr-js19:54
sinzuifab. I can do the review now that I know where the link to your changes are19:54
jtv(also, not a big problem here given how few people use this form)19:54
jtvsinzui: 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
sinzuiThere are examples like that already19:56
jtvphew :)19:57
jtvthe other thing is that the first <li> of the <ul> is getting a little too cozy with the line above the <ul>19:57
sinzuijtv: 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
sinzuiin webkit there is 300px between the form field and the submit buttons. I do not see where the markup error is yet19:58
jtvsinzui: 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
jtvsinzui: 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
sinzuijtv: the error usually indicates that there is a deep error the firefox is not showing you yet.20:01
sinzuijtv: 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 fixed20:02
jtvsinzui: in this case it may be something to do with the existing JS machinery in the form...  nobody would have noticed that, I think20:02
* jtv tries20:03
jtvsinzui: 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
jtvsinzui: oh!  On staging, in konqueror, I get those 300 px or so, right in the middle of the form.20:07
jtvDepending on what I do with that "file type" dropdown20:08
jtvsinzui: so that's a pre-existing bug, very low-priority because of the limited usage of this page.20:08
sinzuiright. The: markup is well formed, but the error implies the table is ill.20:08
jtvIt might just have something to do with the sluggy response I've been getting from firefox on this form.20:09
jtv*sloggy20:09
sinzuijtv: 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 away20:10
jtvsinzui: yay!  thanks20:10
jtvsinzui: no mentor approval needed?20:11
jtv"sluggish," _that_ was the word20:12
sinzuiAnther 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
bachi sinzui: https://code.launchpad.net/~bac/launchpad/bug-490521/+merge/1913320:16
sinzuijtv; the broken edit icon is insane markup, not javascript20:23
jtvsinzui: what went wrong?20:23
sinzuiThe 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 seeing20:25
jtvsinzui: wow, no idea how that got there... unless it was as a way to hide the icon from non-admins.20:26
sinzuiWe created it for diagnostic testing of link rendering20:27
jtvsinzui: 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 bug20:28
sinzuijtv: the issue is a little more complex that that. I will find the bug in rosetta and update it with the correct fix20:28
sinzuijtv: I do not see any javascript20:29
sinzuijtv: is something rewriting the link?20:29
* sinzui see the template and markup look like simple html20:30
jtvsinzui: no, it's done in TAL after all... there's a doctest using the invisible link though.20:31
sinzuijtv: oh, the fix is trivial, it is the white-space inside the anchor that is wrong20:31
sinzuiintellectronica: had me fix a similar issue two weeks ago20:32
sinzuijtv: 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">Change20:32
sinzui                         this entry</span></a>20:32
sinzui                    </tal:block>20:32
jtvsinzui: DON'T tell me the CSS does something with :first here...20:34
sinzuiI am unsure what the issue bug the fix is trivial:20:36
jtvno whitespace between the <a> and the <span>.  Gotcha.20:37
sinzuidude!20:37
sinzuispan.invisible {left:-9999em;display:block;}20:37
jtv?20:38
sinzuiThe link was pushed passed the phone on my desk20:38
sinzuisorry. looking at the wrong entry: span.invisible-link {display:none;} is as I would expect20:39
* jtv was wondering since when css classes had inheritance :-)20:39
jtvsinzui: I'm filing a bug for this... thanks, because I can confidently say I would not have figured this out20:40
sinzuibac: I think your template is missing the display of translations20:51
jtvsinzui: was that meant for me?20:51
bacjtv: i think it was for me20:53
sinzuijtv: 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.png20:53
bacjtv: at least my bell went off20:53
jtvah ok, just checking because of the T word and proximity to conversation with YT.  :)20:53
sinzuiIf the source package has translation templates, then we need to check if auto imports were enabled for the series branch20:54
sinzuijtv: I pondered the confusion as I was typing it20:54
bacsinzui: ok.20:55
sinzuibac: this might work: http://pastebin.ubuntu.com/374223/20:59
jtvsinzui: I don't suppose you'd also like to review this one?21:04
sinzuiyour branch from a few weeks ago?21:04
jtvsinzui: no, this fix you just gave me21:06
jtvsinzui: mainly interesting for the before/after pictures: https://code.launchpad.net/~jtv/launchpad/bug-520659/+merge/1914021:06
sinzuijtv r=me21:07
jtvsinzui: thanks :)21:07
sinzuijtv: I think we can ignore konquerer issues when Lucid is released because the default will be webkit-based21:07
jtvsinzui: for this, that means we can start ignoring konqueror issues when lucid is in late beta.  :-)21:08
sinzuijtv: there are about 20 such bugs we can mark as wont fix21:09
jtvsinzui: so the support for konqueror is basically, "whatever is current in ubuntu"?21:12
sinzuiI think we need to take traffic in consideration...IE6 still haunts US. konqueror is small ans appears to follow the latest release21:13
sinzuijtv: 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
jtvsinzui: I'm surprised about epiphany21:15
sinzuinot me, gecko was always a pain to build and keep up with. gecok was created for the mozilla stack, not for embedding21:16
jtvI suppose a mature but "fresh" engine without too much historical baggage has its attractions...21:18
jtvas ddaa remarked the other day, things would be very different if xhtml had arrived in a time like this when browsers are actually evolving21:18
* jtv -> afk21:19
=== jtv is now known as jtv-afk
bacsinzui: more better?  http://people.canonical.com/~bac/upstream-portlet.png21:22
sinzuiwow, yes21:23
sinzuibac: 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 devel21:25
bacsinzui: so you're talking about the edit link on the series, correct?21:27
sinzuibac: 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
sinzuibac: yes, that was editing the link, but it was not clear that you could change the project and series.21:28
sinzuioh, and I lost the icon in a past revision. I suck21:29
bacsinzui: so change "Show upstream links" to "Change upstream link" and put it to the right of "View changelog"?21:31
sinzuiumm, no21:31
bacyeah, you've lost me then21:31
sinzuiThe link in the horizontal list is goes to the right place, but it is missing an icon (i) -- surely that is my mistake21:32
sinzuibac: 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 wrong21:34
bacsinzui: ring ring21:34
bacthe mic works21:35

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!