=== abentley1 is now known as abentley
=== abentley1 is now known as abentley
=== ursula is now known as Ursinha
=== mrevell is now known as mrevell-lunch
=== matsubara-afk is now known as matsubara
=== mrevell-lunch is now known as mrevell
=== sinzui changed the topic of #launchpad-reviews to:  on-call: - || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== henninge changed the topic of #launchpad-reviews to:  on-call: henninge || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[14:16]  * henninge forgot what "Monday" means ... ;)
[14:28] <henninge> sinzui: ping
[14:28] <sinzui> hi henninge
=== henninge changed the topic of #launchpad-reviews to:  on-call: henninge || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[14:28] <henninge> sinzui: I am looking at the obsolete-styles branch.
=== abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[14:30] <henninge> sinzui: I am not sure about this kind of indention: http://paste.ubuntu.com/331726/
[14:30] <sinzui> I am
[14:30] <sinzui> I am sure that is the best indentation
[14:30] <henninge> sinzui: ;)
[14:31] <sinzui> henninge: CSS is like C, there is no indention, so we choose what we like. I like Python
[14:31] <henninge> sinzui: I was about to ask if that is the goal.
[14:31] <sinzui> henninge: That is also the indentation in style-3.0.css
[14:32] <henninge> sinzui: I see
[14:32] <henninge> sinzui: should style.css be fixed eventually or will it go away, anyway?
[14:33] <sinzui> It should go away
[14:33] <henninge> ok
[14:33] <sinzui> once this branch lands, We can move the official global styles to the new CSS. We then have to decide what to do about the locale rules in the old stylesheet.
=== salgado is now known as salgado-lunch
[14:48] <henninge> sinzui: I see added vertical space between the footer and the content on branch and translations pages. It looks like this is now the same as on the overview page and I think it looks good. Was that intentional?
[14:49] <sinzui> Yes. There were a few pages that were normalised by this
[14:49] <henninge> cool
[14:50] <mrevell> Hey, anyone want to take a look at the what's new branch? The diff is tiny -- https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-3111/+merge/15426
[14:51] <henninge> abentley: ^
=== abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: sinzui, mrevell || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[14:53] <abentley> mrevell: r=abentley
[14:53] <mrevell> thanks abentley!
=== abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: sinzui, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[14:56] <henninge> sinzui: r=me
=== henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[14:56] <sinzui> henninge: thanks.
=== matsubara is now known as matsubara-lunch
=== danilo_ is now known as danilos
[15:17] <adiroiban> henninge: regarding MP reviews, when adding a new MP, should I ping someone, or it is ok just to add them in LP ?
=== beuno is now known as beuno-lunch
[15:28] <henninge> adiroiban: it is always a good idea to ask here.
[15:29] <henninge> adiroiban: the rule is: *You* are responsible for getting a review for your branch.
[15:29] <adiroiban> henninge: ok. then I should ping the current reviewer and ask him/her to look to my MPs ?
[15:29] <henninge> adiroiban: which is me, so you already did ... ;)
[15:30] <adiroiban> yep. One of my MPs is handled by Danilo
[15:30] <henninge> adiroiban: how many are there?
[15:31] <adiroiban> henninge: right now I got 4... for trivial bugs
[15:31] <henninge> adeuring: I see two, bug-135008 and bug-97293
[15:31] <henninge> wow, these are old ;)
[15:32] <henninge> adiroiban: ^
[15:32] <henninge> adeuring: tschuldigung
[15:32] <adeuring> henninge: kein problem :)
[15:32] <henninge> adiroiban: which of these will be reviewed by danilo?
[15:33] <adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-487970/+merge/15250
[15:33] <adeuring> adiroiban: sorry, I failed bady on Friday merging your branch :( I'll try it again when PQM opens again
[15:33] <adiroiban> this one is reviewed by Adel https://code.edge.launchpad.net/~adiroiban/launchpad/bug-193750/+merge/15292
[15:34] <adiroiban> but there are other 2 without a reviewer
[15:34] <adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-135008/+merge/15347
[15:34] <adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-97293/+merge/15391
[15:34] <henninge> adiroiban: I'll take one and slap eon on the queue. Maybe abentley will take one, too.
=== henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: adiroiban, - || queue [adiroiban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[15:36] <henninge> oh, that one is trivial
[15:36] <adiroiban> henninge: yep. all of them are trivial :p 
[15:38] <henninge> adiroiban: are you familiar with YUI?
[15:38] <adiroiban> nope
[15:38] <abentley> henninge: Which one are you doing?
[15:39] <henninge> abentley: 97293, bug I can do both if you are busy, they are quite small.
[15:39] <abentley> henninge: No, I'm not busy.
[15:39] <henninge> ;)
[15:40] <henninge> adeuring: instead of document.getElementById we use Y.get( '#...')
[15:40] <henninge> adiroiban: ^
[15:41] <adiroiban> henninge: ok. I can do that. I was just looking at selectWidget function from the same file
[15:41] <henninge> or maybe that should be Y.one in yui3?
[15:41] <henninge> adiroiban: yes, this code is older than the introduction of yui3
[15:41] <adiroiban> henninge: selectWidget
[15:43] <henninge> adiroiban: let me look at the full code ...
=== salgado-lunch is now known as salgado
[15:48] <henninge> adiroiban: yeah, this whole file would need to be converted to use yui. So never mind yui3 for now...
[15:49] <adiroiban> henninge: i'm still learning about LP dev practice. In the future I will try to use YUI3
[15:50] <henninge> adiroiban: that is good! but in this case not using it probably was the better choice, to keep the code consistent.
[15:51] <abentley> henninge: Do you know why there's a search_lang attribute on LanguageSet?  It doesn't seem to be part of the model.
[15:52] <adiroiban> abentley: search_lang is the HTTP_GET argument . not part of the model
[15:52] <adiroiban> abentley: it is only relevant for the form. Is the string used to match language names while doing the search
[15:53] <adiroiban> abentley: maybe I did something stupid, and „search_lang” should have been defined in another place
[15:53] <henninge> abentley: Let me look at it.
[15:53] <abentley> adiroiban: If it is only relevant for the form, why is it on the ILanguageSet interface?
[15:54] <abentley> henninge: It's adiroiban's addition, I see now.
[15:55] <adiroiban> abentley: i didn't know where it needs to be defined... so i put it there
[15:56] <adiroiban> abentley: can you suggest me how it should have been implemented? I can fix that :)
[15:56] <abentley> adiroiban: I suggest defining a new interface called ILanguageSearch in lib/lp/translations/browser/language.py
[15:57] <adiroiban> abentley: ok. you can leave all your observation as a comment in MP and I will try to solve those requests
[15:59] <abentley> adiroiban: Since ILanguageSearch will have only the member you want, you won't need the field_names list.
=== matsubara-lunch is now known as matsubara
[16:01] <abentley> adiroiban: What was your motivation for changing LanguageSetView into a LaunchpadFormView?
=== abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: adiroiban, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:02] <adiroiban> abentley: i was thinking this is the „standard” way of adding JS autoselect entry to a form
[16:04] <danilos> adiroiban, hey, thanks for all the work, I see you are coding on steroids :) I've reviewed your branch, do note that we won't be able to land any one of them before Wednesday, and most likely not before next Monday (we are only allowing release-critical landings right now)
[16:04] <adiroiban> abentley: I looked at all the other forms that have auto-focus enabled and they were all using LaunchpadFormView
[16:05] <abentley> adiroiban: Why do you tal:define script and use script in tal:content, instead of doing tal:content="view/focusedElementScript" ?
[16:05] <henninge> adiroiban: r=me
=== henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: -, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:06] <henninge> adiroiban: you will have to wait until next week to land it, though, as this will not be considered release-critical.
[16:06] <adiroiban> abentley: at is was use both in tal:condition and tal:content
[16:06] <henninge> adiroiban: please ping danilo or me on Monday to do it for you.
[16:07] <adiroiban> abentley: as is was use both in tal:condition and tal:content. I can change it if required
[16:07] <abentley> adiroiban: I am asking questions so I understand your patch better.  I'm not implying your choices are wrong.
[16:08] <adiroiban> abentley: np. 
[16:09] <abentley> adiroiban: So the only problem I see is the first one: putting search_lang on ILanguageSet.  The rest looks fine, now that I understand it.
[16:10] <adiroiban> abentley: ok. I will fix that.
[16:11] <bac> henninge, abentley: could one of you have a look at https://code.edge.launchpad.net/~bac/launchpad/ec2_attached/+merge/15272
[16:11] <henninge> bac: looking
[16:16] <abentley> adiroiban: The Interfaces in our "interfaces" directories are meant to correspond with model objects.  ILanguageSet shouldn't specify anything that LanguageSet can't provide, which is why search_lang didn't belong on ILanguageSet.  Does that make sense?
[16:17] <adiroiban> abentley: yep. I was not aware of that policy
=== henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: bac, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:17] <henninge> bac: stray blank line at line 8 of the diff.
[16:17] <danilos> henninge, adiroiban: re bug-97293, there's a jslint warning: was that there before as well? if it was, never mind it, but if it wasn't, you should fix it first :)
[16:18] <henninge> danilos: oops, missed that
[16:18] <adiroiban> danilos: it was there
[16:18] <adiroiban> i did not touched that line 
=== abentley changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: bac, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:18] <danilos> adiroiban, ok, if you can figure out how to fix it, that'd be great, if not, never mind :)
[16:18] <adiroiban> danilos: I have no idea how to fix it 
[16:19] <danilos> adiroiban, ok, then just ignore it (or perhaps file a bug against launchpad-foundations)
[16:19] <bac> henninge: gone
[16:20] <adiroiban> danilos: i can read about JS closures and try to fix it ... if I will fail, will file a bug
[16:21] <henninge> bac: I am surprised there are not more parameters to the Option constructor but I guess boolean values defaulting to False are the default, right?
[16:21] <danilos> adiroiban, I think this is best left for whoever made it such, the code where it happens does look ugly with a debugging function being defined in a loop
[16:21] <bac> henninge: i can double-check that
[16:22] <henninge> bac: the Option 'force' above that looks the same ...
[16:22] <bac> henninge: yes, that was my example
[16:22] <danilos> adiroiban, it's not really a function definition, it's a YUI3 way of doing things, so I just wouldn't worry about it :)
[16:22] <henninge> danilos: I agree.
[16:22] <adiroiban> danilos: ok. then no need to file a bug?
[16:23] <danilos> adiroiban, you can if you want to, but don't spend time trying to fix it :)
[16:23] <adiroiban> danilos: ok
[16:23] <henninge> bac: where does run get called? I see the extension of the parameter list but would expect that to happen at the call site, too.
[16:25] <henninge> bac: oh, it's from EC2Command
[16:25] <bac> henninge: let me look.  it's part of the 'commandant' machinery
[16:25] <henninge> bac, yes, I just just figured.
[16:26] <henninge> bac: r=me
[16:27] <bac> thanks henninge.
=== henninge changed the topic of #launchpad-reviews to:  on-call: henninge, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:30] <adiroiban> abentley: regarding the addition of LanguageSearch interface. Should I put it in lp.translations.interfaces.language rather than lp.services.worlddata.interfaces.language ?
[16:32] <abentley> adiroiban: As you explained, it has no purpose outside the context of the view.  Therefore, I suggest putting it next to the view.  I don't think it has value as a public API.
[16:33] <adiroiban> abentley: next to view means putting the interface in the same file ?
[16:34] <abentley> adiroiban: Yes.
[16:34] <adiroiban> abentley: :) ok
[16:35] <abentley> I specified that file here and in the review.
[16:48] <adiroiban> abentley: I have pushed the required changes. What would be the next step?
[16:49] <abentley> adiroiban: I'll have a look.
[16:50] <adiroiban> abentley: what is the normal LP process. Should I add a comment, ping you... or both ?
[16:51] <abentley> adiroiban: Normally, you'd add a comment saying you've made the requested changes.
[16:51] <abentley> adiroiban: Why have you added ILanguageSetSearch to lib/lp/services/worlddata/interfaces/language.py ?
[16:52] <adiroiban> abentley: it should be an error.
[16:52] <abentley> adiroiban: I don't understand.
[16:52] <adiroiban> abentley: it's just me being stupid . I'll fix that right away
[16:53] <abentley> adiroiban: Okay.
[17:00] <abentley> adiroiban: This looks good to me now.
[17:00] <adiroiban> abentley: sorry for messing it 
[17:00] <abentley> adiroiban: np
[17:03] <adiroiban> abentley: ok. Should i ping you on Monday, or everthing is ok?
[17:04] <abentley> adiroiban: Please ping me on Monday.
[17:05] <adiroiban> abentley: ok. Henning  told me I responsible for getting a review for your branch. I am also responsible for getting it merged into devel ?
[17:06] <abentley> adiroiban: I'm not sure.  This is the first patch I've handled from a non-committer.
[17:06] <adiroiban> abentley: ok.
[17:07] <abentley> adiroiban: It would be simpler if this wasn't week 4.
[17:10] <henninge> abentley, adiroiban: I'd say you are responsible as long as non of us says "I'll handle it from here on" ;)
[17:11] <henninge> adiroiban: So get back to us about it in a week and we'll take the landing off your hand.
[17:11] <adiroiban> henninge: ok. no hurry to have them accepted/merged. I just wanted to learn the workflow
[17:11] <henninge> abentley: you can just land it, it will be credited to Adi.
[17:12] <abentley> adiroiban: Have you signed the contributor agreement?
[17:12] <henninge> adiroiban: we are in feature-freeze this week because of the roll-out, that is the only reason for not going straight ahead.
[17:12] <adiroiban> abentley: yep
[17:12] <adiroiban> abentley: you can check here https://edge.launchpad.net/~contributor-agreement-canonical
[17:13] <henninge> adiroiban: It *may* be best to have one person land all your branches.
[17:13] <henninge> but I am not sure.
[17:13] <adiroiban> henninge: ok. in that case, what is the next action I will have to do ?
[17:15] <henninge_> adiroiban: The next step is run the full test suite on it (on ec2) and land the branch
[17:15] <henninge_> adiroiban: oh, and since a week will pass, wou will also have to merge devel again to check for conflicts.
[17:15] <adiroiban> henninge_: hm...  
[17:16] <adiroiban> henninge_: i can do the devel merge... but not sure about ec2 testing
[17:16] <henninge_> adiroiban: once that is done (and resolved), ping me and I'll do the testing and landing.
[17:16] <henninge_> adiroiban: no, you cannot.
[17:17] <adiroiban> henninge_ ok.
[17:17] <henninge_> adiroiban: you *could* run the test suite locally (make check), that'll take a few hours, but there is no need for that.
[17:18] <adiroiban> henninge_: on my computer it will take days :)
[17:18] <adiroiban> henninge_: but I'm running the translation test suite
[17:18] <adiroiban> and also testing for specific stories
[17:19] <henninge_> cool
[17:19] <henninge_> adiroiban: I will go home now. ttyl
[17:20] <adiroiban> henninge_: have a nice evening
=== henninge_ changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[17:20] <henninge_> adeuring: you too!
[17:21] <adeuring> henninge: ...and you ;)
=== beuno-lunch is now known as beuno
=== abentley is now known as abentley-lunch
=== deryck is now known as deryck[lunch]
=== abentley-lunch is now known as abentley
=== abentley1 is now known as abentley
=== deryck[lunch] is now known as deryck
[20:07] <deryck> abentley, I'm about to go EOD, but I have a really simple css fix.  care to take a look, or can you do an offline review later?
[20:13] <abentley> deryck: I'll do an offline review later.
[20:14] <thumper> deryck: I've grabbed the branch
[20:14] <thumper> deryck: and happy to look at it
[20:14] <deryck> thumper, excellent, thanks!
[20:14] <deryck> thanks anyway, abentley (since thumper has it now)
[20:36] <deryck> thumper, I've got to bail for the day.  Will check back later about the review.
[21:17] <bac> abentley: i've got a branch coming up which is targeted to the next cycle.  would you have time to look at it?
[21:17] <abentley> bac: probably.  on a call.
[21:18] <bac> abentley: ok.  i'll paste it when it shows up.  no rush.  thanks.
=== salgado is now known as salgado-afk
[21:28] <bac> abentley: when you have time: https://code.edge.launchpad.net/~bac/launchpad/bug-429802/+merge/15449
=== matsubara is now known as matsubara-afk
=== abentley changed the topic of #launchpad-reviews to:  on-call: abentley || reviewing: bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[21:55] <mars> abentley, rc-ping, would you feel comfortable reviewing the JavaScript for https://code.edge.launchpad.net/~mars/launchpad/fix-picker-result-parsing-487975/+merge/15436?  39 lines.
[21:57]  * thumper looks
[22:00] <abentley> mars: There seems to be an extra space before the = in line 9 of the diff.
[22:01] <abentley> mars: Aside from that, it looks fine to me.
[22:02] <mars> abentley, nice catch.  Thanks for the review.
[22:02] <abentley> mars: np
=== abentley changed the topic of #launchpad-reviews to:  on-call: - || reviewing: bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[22:04] <thumper> abentley: heh, I said exactly the same thing :)
[22:09] <mars> thumper, thank you too.  I'll send off this branch, then finish up your reply
[22:09] <thumper> mars: ok
[22:11] <mars> thumper, the question I had about creating a new wrapper was the commit_message_listener function on line 25.
[22:12] <thumper> mars: I still feel like I'm missing something
[22:12] <mars> thumper, that has a green and a red flash on the link.  The current editor wrapper flashes the message itself.  I take it this is an extra, additional bit of flash?
[22:12] <thumper> mars: ah
[22:13] <thumper> mars: this is flashing the menu link not the editor itself
[22:13] <thumper> mars: if the commit message ends up being empty
[22:13] <thumper> mars: we hide the multiline editor
[22:13] <thumper> mars: show the link again, and flash the link
[22:13] <mars> ah
[22:13] <mars> ok
[22:14] <thumper> mars: it would be nice to be able to use widget.on('save')
[22:14] <thumper> mars: rather than widget.editor.on('save')
[22:14] <thumper> mars: is this possible, because at the moment it doesn't work
[22:14] <mars> yep
[22:14] <mars> it didn't work in PR2.  It probably works fine in 3.0 final.
[22:15] <mars> rather, that feature didn't work in PR2 - I tried to code it already, and gave up :)
[22:15] <flacoste> mars: what is that?
[22:15] <flacoste> that's parsing the HTML representaion?
[22:15] <flacoste> why is it doing that
[22:15] <flacoste> we shouldn't be doing that
[22:15] <mars> flacoste, one of many approaches I take it.
[22:15] <flacoste> you should be requesting the field HTML output directly
[22:16] <flacoste> that's a deprecated approach
[22:16] <mars> this is an original widget
[22:16] <flacoste> we talked about making that view a 404
[22:16] <flacoste> to fix that broken behaviour
[22:16] <flacoste> and it's the third time that somebody "fixes" it
[22:16] <mars> good thing you didn't then :)
[22:16] <flacoste> that's crazy
[22:17] <thumper> mars: it doesn't work in 3.0 right now with the widgets we have
[22:17] <mars> thumper, yep, I know.  I would have to add it back into lazr-js, then publish that new version in LP before you could use it.
[22:17] <thumper> ok
[22:18] <mars> thumper, so maybe in a release or two.  Your branch did show some serious API problems though.  I like that.
[22:19] <mars> flacoste, is there a bug for "Don't use the XHTML view" I can reference with an XXX?
[22:19] <flacoste> probably, but it will have been marked as 'Fix released'
[22:19] <mars> flacoste, IIRC publishing instructions for the Right Way To Do It is on the UI Action items list
[22:20] <mars> ok, I'll exhume the bug
[22:36] <mars> flacoste, I can't find the bug number for XHTML representations.  I'll just file a new one and mark it as an XXX.
[22:38] <mars> flacoste, anything else needed for an r-c?
[22:39] <flacoste> mars: nope, just updated the m-p
[22:39] <mars> thanks
[22:42] <thumper> mars: so want to approve the js review? https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260
[22:42] <mars> thumper, doing so now
[22:42] <thumper> mars: thanks
[22:43] <thumper> salgado-afk (or flacoste): https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260 rc?
[22:48] <mars> thumper, two small fixes, r=mars
[22:48] <thumper> mars: ok, ta
[22:51] <mars> thumper, to clarify the code, a quick comment with the same reason you gave me on IRC should be enough.
[23:38] <flacoste> thumper: rc=me
[23:46] <thumper> flacoste: ta