| thumper | mwhudson: ping | 02:22 |
|---|---|---|
| mwhudson | thumper: pong | 02:22 |
| thumper | https://code.edge.launchpad.net/~thumper/launchpad/defer-wip-email/+merge/38791 ? | 02:22 |
| lifeless | henninge: I've done config.isTestRunner - there was another site that needed updating, so refactor + JFDI | 02:23 |
| mwhudson | thumper: looking | 02:27 |
| mwhudson | thumper: i was sure i reviewed that yesterday :/ | 02:28 |
| mars | StevenK, online? | 03:26 |
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| mars | jtv, was just going to point out to StevenK that I saved a bunch of reviews for him in the +activereviews queue | 03:26 |
| jtv | mars: I get a vague sense that you're hinting at something | 03:27 |
| jtv | It must be lack of coffee. | 03:27 |
| mars | jtv, well, if he is an OCR mentee today, then perhaps you and he can work out some way to split the workload so that he can gain more experience, but not be overwhelmed with the volume | 03:28 |
| jtv | mars: I don't see anything of yours under Launchpad's "requested reviews I can do"… | 03:28 |
| mars | jtv, I was OCR last shift - I left the reviews on the +activereviews queue so StevenK could pick some of them up | 03:29 |
| jtv | ah so | 03:29 |
| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| thumper | jtv: hi | 03:57 |
| jtv | hi thumper | 03:58 |
| jtv | is this about reviews? | 03:58 |
| thumper | jtv: trivial one for you: https://code.edge.launchpad.net/~thumper/launchpad/kill-bad-form-preloads/+merge/38800 | 03:58 |
| thumper | jtv: yes... yes it is | 03:58 |
| jtv | I hate that word: "trivial" | 03:58 |
| jtv | because it never is. | 03:58 |
| thumper | sure it is | 03:58 |
| jtv | Growing JS in the TAL, I see. | 04:00 |
| jtv | thumper: is the LP.client.links['me'] !== undefined check something standardized? | 04:03 |
| thumper | jtv: used elsewhere | 04:03 |
| thumper | I'm just using it again | 04:03 |
| thumper | standardized? prolly not | 04:04 |
| jtv | This is something I don't like about "undefined": what if someday it becomes a null? Will we notice that this code breaks? | 04:04 |
| thumper | jtv: only with a shed load more 404s :) | 04:05 |
| thumper | jtv: like we get now | 04:05 |
| jtv | :/ | 04:06 |
| jtv | I know our coding guidelines like explicit comparisons, but it'd be so much more robust to test if (!foo)… | 04:06 |
| jtv | And adding 2 windmill tests for this is a hefty price to pay as well. Crap. | 04:09 |
| jtv | thumper: have you considered solving the problem in TAL? | 04:11 |
| thumper | which problem? | 04:12 |
| thumper | the logged in one? | 04:12 |
| thumper | no, not really considered it at all | 04:12 |
| jtv | I was wondering if we could have a marker class on #portlet-subscribers, so that there's simply nothing to hook the lazr widget into if it's not appropriate to show it. | 04:15 |
| jtv | The marker class would appear only if it's appropriate to show the widget. | 04:16 |
| jtv | When the marker class is absent, installing the widget AIUI should do nothing because it won't find the bit of HTML that it's trying to hook into. | 04:16 |
| wgrant | StevenK: Is there an existing constant for that? | 04:25 |
| wgrant | I couldn't find one. | 04:25 |
| StevenK | wgrant: There could be, I meant to add "i'm not certain" | 04:29 |
| StevenK | Perhaps move the constant to same places as the others? | 04:29 |
| wgrant | StevenK: Ah. I guess the "It's" could have been "Is" or "Isn't", and I read it as the latter. | 04:29 |
| wgrant | I could. | 04:29 |
| wgrant | IMHO it belongs next to the publisher. | 04:30 |
| wgrant | But I could be wrong. | 04:30 |
| jtv | thumper: I think my modem wanted to be alone for a while. What was the last you saw of me? | 04:30 |
| thumper | <jtv> When the marker class is absent, installing the widget AIUI should do nothing because it won't find the bit of HTML that it's trying to hook into. | 04:42 |
| thumper | but I wasn't listening :) | 04:42 |
| jtv | thumper: that's a shame, because leaving this free to regress untested isn't great either, and two windmill tests would be overkill. | 04:43 |
| StevenK | thumper: I've got two mentor reviews I've just subscribed you too if you have a few seconds. | 04:43 |
| thumper | StevenK: ack | 04:43 |
| thumper | jtv: I'll think on it and discuss with my JS expert, rockstar | 04:44 |
| * rockstar huhs | 04:44 | |
| jtv | thumper: sure. What I'm suggesting is not going to be as pretty as it sounds, because pure TAL won't help you inject conditional classes. So a bit of help from the view would probably be needed. | 04:45 |
| jtv | rockstar: hi | 04:45 |
| rockstar | hi jtv. Am I up that late? | 04:45 |
| jtv | rockstar: yup. :) | 04:45 |
| jtv | We're just talking about this MP: https://code.edge.launchpad.net/~thumper/launchpad/kill-bad-form-preloads/+merge/38800 | 04:45 |
| rockstar | Ooh, I hope this is what I think it is. | 04:45 |
| jtv | It conditionally disables subscription widgets if the user is not logged in. | 04:45 |
| jtv | But that's awkward to test (windmill! I'm Dutch and I don't even like to work with it) | 04:46 |
| jtv | and currently untested, which I don't care much for either. | 04:46 |
| rockstar | jtv, that's okay, since the subscription widget works without javascript. | 04:46 |
| rockstar | So it only disables the progressive enhancement. | 04:46 |
| jtv | Yes. | 04:47 |
| rockstar | Although I guess we could teach the widget to look for lp.me, and if it doesn't exist, don't create the activator for the form overlay. | 04:47 |
| jtv | That'd be good too. After all, nobody who isn't logged in has any business editing anything at all in LP. | 04:48 |
| jtv | Is this an LP widget or a lazr widget? | 04:48 |
| rockstar | jtv, it's an LP widget that inherits from a lazr widget. | 04:53 |
| jtv | rockstar: then what you say would be ideal | 04:54 |
| rockstar | jtv, yeah, as I look at this patch, I wonder if it's fair to kill all/most javascript for those not logged in. | 04:54 |
| jtv | That's a good question. | 04:55 |
| rockstar | Most of our javascript is for editing anyway. | 04:55 |
| jtv | Definitely not all JS, but the only thing you could sensibly do with the API in JS is log in. | 04:56 |
| rockstar | jtv, thumper's change here is good, methinks. The subscription portlet's progressive enhancement is specifically for handling inline subscriptions. If you're not logged in, why bother with the javascript? | 04:56 |
| jtv | rockstar: I agree with that part. What bothers me is leaving it untested. | 04:57 |
| jtv | But I don't see a justification for several additional windmill tests either. | 04:57 |
| rockstar | jtv, I don't think they're necessary. | 04:57 |
| rockstar | jtv, also, windmill is at the top of my shit list right now. | 04:57 |
| jtv | I hear you. | 04:58 |
| * StevenK points at his thread on -dev about windmill's other problem | 04:59 | |
| jtv | StevenK: which thread is that? | 05:02 |
| StevenK | jtv: 'Failures in Hudson' | 05:03 |
| StevenK | on, even | 05:03 |
| jtv | No wonder I didn't recognize it. :) | 05:03 |
| jtv | I thought it was just people complaining about Michael. :-) | 05:03 |
| StevenK | Bwahaha | 05:03 |
| StevenK | It couldn't be that, mwhudson didn't reply with "How dare you!?" | 05:04 |
| jtv | Oh well, I don't suppose windmill's individual failings are relevant here—we'd probably never finish. | 05:06 |
| StevenK | jtv: And see -dev, since lifeless rocks | 05:06 |
| jtv | rockstar: one final attempt at getting this testable _without windmill_ though. | 05:07 |
| jtv | I was thinking we could add a marker class to the subscription portlet that only shows up if it's appropriate for the user to have the subscription editing widget. | 05:08 |
| rockstar | jtv, that seems to be a hack to get around the fact that our tools are stupid. | 05:08 |
| jtv | More a hack to get around the fact that JS will let you get away with anything and in this case, potentially produce a regression that lots of users will notice but we may not. | 05:09 |
| rockstar | jtv, what's the regression potential here? | 05:11 |
| jtv | It keeps most of the logic in slightly more tightly controlled areas. | 05:11 |
| jtv | The "me" check goes wrong. | 05:11 |
| rockstar | jtv, there's a windmill test already that makes sure that you have the JS when you are logged in. | 05:12 |
| jtv | rockstar: eparse | 05:12 |
| jtv | What does "have the JS" mean? | 05:13 |
| rockstar | jtv, the progressive enhancement has tests for logged in users already. | 05:13 |
| rockstar | So the test you're proposing is merely "create a windmill test that makes sure there ISN'T javascript executed." | 05:14 |
| jtv | As I've been saying again and again, I'm not proposing a windmill test. I hate windmill tests! | 05:16 |
| jtv | I'm saying I don't like logic in places where only windmill can cover it. | 05:17 |
| jtv | The reason being that it can fail visibly without us ever noticing. | 05:18 |
| jtv | Don't we have a plain "user is logged in" function or variable somewhere? | 05:19 |
| jtv | That would address the same problem. | 05:20 |
| rockstar | jtv, yeah, but I don't think the test is going to do any good at all without testing the client (where the fix is occurring) | 05:27 |
| jtv | True. | 05:27 |
| jtv | A "user is logged in" function could at least be unit-tested instead of integration-tested though; if we had one, it'd take just that bit more risk out of the JS. | 05:28 |
| jtv | I'm approving as-is, but it's clear that we have some desirable improvements to the JS infrastructure. | 05:29 |
| rockstar | jtv, considering that this bug only manifests itself as an error in a javascript console that most users won't look at, and that if we find an issue in it, we've still got a test to assert the progressive enhancement works when a user is logged in, I think the risk is pretty small. | 05:30 |
| jtv | I wasn't aware that it wasn't user-visible. | 05:30 |
| rockstar | jtv, yeah, it sounds like the rest of Canonical has jumped the windmill ship already. We're a bit behind on the times. :) | 05:31 |
| jtv | Jumping the windmill ship okay, but where to? We always knew windmill sucked, but at the time there was simply no alternative. | 05:32 |
| rockstar | jtv, landscape is using jstestdriver, and u1 has set a course to jstestdriverland as well. | 05:33 |
| rockstar | And lazr-js already uses jstestdriver | 05:34 |
| jtv | That looks nice. | 05:34 |
| jtv | thumper: I approved your branch but I'm thoroughly disoriented by the UI. | 05:36 |
| jtv | I give an Approved vote for "js." Now I can't add an Approved vote for "code." | 05:37 |
| jtv | So I request another review, from myself, for "code." | 05:37 |
| jtv | Now my js approval shows up as a code approval. | 05:37 |
| bac | yeah, you need to combine them "code js" | 05:38 |
| jtv | Gah. | 05:38 |
| bac | double gah | 05:38 |
| jtv | So now I change the type again by requesting yet another review from myself. | 05:38 |
| bac | you can just add another comment to the review | 05:39 |
| bac | with 'approve' / 'code js' | 05:39 |
| rockstar | bac, do you still need me to chair the meetings tomorrow? | 05:42 |
| bac | yes, rockstar. and i thank you. | 05:42 |
| bac | i meant to remind you but forgot | 05:42 |
| rockstar | bac, do you have a wiki page to point mo to with old bidness? | 05:42 |
| bac | hope the 8am meeting isn't too too early for you | 05:42 |
| bac | rockstar: just the ReviewerMeetingAgenda | 05:43 |
| bac | on dev | 05:43 |
| bac | rockstar: i'll update it this afternoon | 05:43 |
| rockstar | bac, no, I have a 8:10 class every weekday now. | 05:43 |
| bac | in person? | 05:44 |
| bac | can you do your class and the meeting? | 05:44 |
| rockstar | bac, no, but I can miss it. I'm pretty ahead in that class. | 05:44 |
| * bac contributes to truantism | 05:45 | |
| rockstar | It's the Tues/Thurs PM class that I can't miss much more of. | 05:45 |
| bac | you can tell i used to watch "Little Rascals" | 05:45 |
| * bac pedals off in search of lunch | 05:46 | |
| rockstar | Set up canonical.testing.layers.AppServerLayer in 3 minutes 11.145 seconds. | 06:12 |
| rockstar | What. The. Hell. | 06:12 |
| === jtv is now known as jtv-eat | ||
| === jtv-eat is now known as jtv | ||
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: mvogt || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | henninge: i've made changes to my branch and need a UI review. can you give it a look? screenshots are attached. https://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/38705 | 09:05 |
| bac | henninge: also, you mentioned the edit icon being misplace, but i'm rendering that link as done elsewhere | 09:05 |
| henninge | bac: otp now but will look at it later. | 09:06 |
| bac | henninge: thanks! | 09:06 |
| thumper | StevenK: ping | 09:14 |
| thumper | StevenK: I'll go over https://code.edge.launchpad.net/~wgrant/launchpad/no-lucilleconfig-directories/+merge/38650 once wgrant has got back to you and you've approved the review | 09:17 |
| jtv | bigjools: I'm looking at mvogt's branch from a code-review perspective. Have you looked at the design and if so, any problems with it from that angle? | 09:21 |
| StevenK | thumper: Sorry, I was out walking. I've commented on it. | 09:21 |
| bigjools | jtv: what branch? (can you tell I know nothing? :)) | 09:22 |
| jtv | bigjools: https://code.edge.launchpad.net/~mvo/launchpad/support-timeframe-fix-660433/+merge/38503 | 09:22 |
| bigjools | jtv: that is not ready for review, I need to evaluate it first | 09:22 |
| thumper | StevenK: but you haven't approved it, I'll look then | 09:23 |
| thumper | StevenK: it seems to be good for now | 09:23 |
| thumper | I've got nothing extra to add | 09:23 |
| StevenK | thumper: Sorry, let me do so. | 09:23 |
| thumper | StevenK: are you happy with it now? | 09:23 |
| bigjools | jtv: I told him I'd need to test it on dogfood before it got further, I didn't know there was an MP because it's not linked to the bug | 09:24 |
| jtv | bigjools: I'll fix that | 09:24 |
| jtv | bigjools: if convenient, I'd be grateful if you could have a look so that I'll be free vote Approved if appropriate. | 09:25 |
| bigjools | jtv: sure thing, after my standup in 30 mins | 09:25 |
| jtv | cool | 09:26 |
| jtv | linked. | 09:26 |
| StevenK | thumper: I've approved it | 09:27 |
| bigjools | jtv: FWIW I don't like the mocks that live in files all over the place | 09:27 |
| jtv | bigjools: I noticed somebody moved some of our test-only objects into main code... | 09:27 |
| bigjools | jtv: ? | 09:27 |
| StevenK | bigjools: 25 minutes until the stand-up? | 09:29 |
| bigjools | StevenK: no :) | 09:29 |
| thumper | StevenK: approved too | 09:29 |
| StevenK | thumper: Danke | 09:29 |
| thumper | np | 09:29 |
| jtv | bigjools: I thought that was what you meant with "mocks that live in files all over the place." | 09:30 |
| bigjools | jtv: no, he's created a ton of new directories with weird stuff (TM) | 09:30 |
| * jtv likes Weird Stuff | 09:30 | |
| bigjools | cronscripts/publishing/tests/mock-data/mock-bin/germinate for example | 09:30 |
| jtv | And by the way, <compose>tm | 09:31 |
| bigjools | that does nothing here | 09:31 |
| jtv | Isn't that the part where he also noted this himself, and cleaned it up later? | 09:31 |
| bigjools | jtv: it's in the current diff | 09:31 |
| * bigjools -> standup | 09:31 | |
| bigjools | back in a bit | 09:31 |
| jtv | Ah there | 09:31 |
| jtv | OK | 09:31 |
| bigjools | jtv: I asked cjwatson to take al ook | 10:23 |
| bigjools | a look, too | 10:23 |
| jtv | Well a good Al Ook is nice too. | 10:23 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jtv, jelmer || Reviewing: mvogt || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jtv changed the topic of #launchpad-reviews to: On call: jtv, jelmer || Reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | jelmer: we've got 2 more on the queue, but I'm having a call soon followed by EOD. | 11:16 |
| jelmer | jtv: Ah, ok | 11:18 |
| jelmer | jtv: I'll have a call soon as well, but I'll take a look at wgrants branch first. | 11:18 |
| jtv | jelmer: great, thanks | 11:18 |
| jtv | I'll see if I can squeeze in Ian's. | 11:18 |
| wgrant | jelmer: Thanks. | 11:18 |
| === jtv changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | jelmer: I'll still try to review that other branch, just not open for OCR business any more for the day. | 11:26 |
| jtv | jelmer: I'm afraid I won't be able to review Ian's branch tonight, sorry! | 12:06 |
| henninge | bac: ui review done. Sorry for the delay, I was hunting misalignments ... | 12:06 |
| jelmer | jtv: No worries; I'll have a look after I finish reviewing William's branch. | 12:07 |
| === matsubara-afk is now known as matsubara | ||
| === Ursinha-afk is now known as Ursinha | ||
| * jelmer lunches | 13:15 | |
| deryck | hi allenap. wb! | 14:08 |
| deryck | allenap, can we get qa done for bug 655567? | 14:08 |
| _mup_ | Bug #655567: Wire up getSubscriptionsForBug() into the notifications machinery <qa-needstesting> <story-subscribe-to-search> <Launchpad Bugs:Fix Committed by allenap> <https://launchpad.net/bugs/655567> | 14:08 |
| allenap | deryck: Sure. | 14:09 |
| deryck | allenap, thanks. I meant to ask on #launchpad-dev. irc client tab fail. :-) | 14:09 |
| deryck | jelmer, hi. I have one for review. Can I get in line on the queue? | 14:10 |
| jelmer | deryck: yes, of course | 14:10 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: wgrant || queue: [deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| deryck | jelmer, thanks! https://code.edge.launchpad.net/~deryck/launchpad/lock-fix-released-status-126516/+merge/38931 | 14:10 |
| leonardr | jelmer, i request a review of https://code.edge.launchpad.net/~leonardr/launchpadlib/remove-broken-code/+merge/38935 | 14:45 |
| jelmer | leonardr: Wow, that removes a lot of code.. I'll have a look after Deryck's branch. | 14:46 |
| * leonardr regrets ever writing that code in the first place... should have stuck to my guns | 14:47 | |
| === leonardr changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: wgrant || queue: [deryck,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === adeuring1 is now known as adeuring | ||
| === abentley_ is now known as not-abentley | ||
| === matsubara is now known as matsubara-lunch | ||
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: deryck || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === salgado is now known as salgado-lunch | ||
| jelmer | deryck: Hi | 15:56 |
| deryck | hi jelmer | 15:56 |
| jelmer | deryck: Is your MP linked to the right bug? As I read it the bug report asks for disallowing unprivileged users to not be allowed to set the bugstatus to FIXRELEASED, whereas the branch (as I understand it) disallows them from moving away from FIXRELEASED. | 15:58 |
| deryck | jelmer, well, it's the bug I claimed as being for my work, understanding it wasn't a perfect fit. See my last comment on the bug. but I certainly could have misread the bug. | 15:59 |
| * jelmer apologizes for his bad English.. there's an unnecessary use of the word allow in that senstence | 15:59 | |
| * deryck looks closer | 15:59 | |
| deryck | jelmer, yes, that bug is not really related as my fix stands now. I was originally going to go the full route of making fix released a bug supervisor status, but then changed my mind. | 16:03 |
| deryck | jelmer, I'd like to land this as an incremental fix then, and think further about allowing FIXRELEASED as a fully limited bug supervisor status. | 16:03 |
| deryck | or I can just unlink the branch and file a new bug. | 16:03 |
| jelmer | deryck: Either seems reasonable, just making sure that bug wouldn't be closed accidentally when this branch lands. | 16:04 |
| deryck | jelmer, yes, thanks. I'll follow up and make sure about that. good catch. | 16:06 |
| abentley | bdmurray: ISTM that NominateBugForProductSeries and NominateBugForDistroSeries vary only in their usedfor attribute. Correct? | 16:08 |
| jelmer | deryck: r=me | 16:08 |
| deryck | jelmer, thanks! | 16:08 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha is now known as Ursinha-lunch | ||
| leonardr | salgado, would appreciate your help on https://code.edge.launchpad.net/~leonardr/launchpad/temporary-integration/+merge/38836 | 16:12 |
| bdmurray | abentley: yes, that's correct. is there another way to do that? | 16:14 |
| abentley | bdmurray: You could have a base class that didn't have a usedfor attribute, and provide NominateBugForDistroSeries and NominateBugForProductSeries as subclasses. | 16:15 |
| === matsubara-lunch is now known as matsubara | ||
| bdmurray | abentley: I think there are some other places I could use that base class too so maybe that'd be better for a separate branch? | 16:16 |
| abentley | bdmurray: Sure. | 16:17 |
| abentley | bdmurray: Around line 45, did you consider using "if check_permission() or check_permission()" instead of if/elif? That would avoid repeating the Link code. | 16:18 |
| rockstar | jelmer, hi. I'd like to hop on your queue. Do you have time? | 16:18 |
| abentley | bdmurray: Oh, I see that the actual text of the link varies. | 16:18 |
| jelmer | rockstar: yep, go ahead | 16:19 |
| === rockstar changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: leonardr || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bdmurray | abentley: right, I consolidated it as best I could | 16:19 |
| abentley | bdmurray: the text variation is because the bug driver's "nomination" is automatically approved? | 16:20 |
| bdmurray | abentley: yes, that is correct | 16:20 |
| abentley | bdmurray: Your change introducing LinkNotFound reminds me that we do not normally disable links for anonymous users, on the assumption that they can log in if they get an Unauthorized error. | 16:22 |
| abentley | bdmurray: around 217, why have you changed specific imports to imports from canonical.launchpad.interfaces? | 16:24 |
| bdmurray | abentley: That might have been from a merge conflict I had. It is better to have the specific imports then? | 16:25 |
| abentley | bdmurray: yes, it is. It reduces the chance of circular import problems, and it reduces our dependence on the deprecated canonical.launchpad namespace. | 16:26 |
| bdmurray | abentley: okay, fixing | 16:26 |
| abentley | bdmurray: I also see that around 68 | 16:28 |
| abentley | bdmurray: sinzui is working to eliminate use of canonical.launchpad: https://code.edge.launchpad.net/~sinzui/launchpad/apocalypse-interface-imports-7/+merge/38727 | 16:30 |
| jelmer | leonardr: r=me | 16:34 |
| leonardr | yay | 16:34 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || Reviewing: rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | deryck: our usual pattern is to show all links to anonymous users, and if they get Unauthorized, they can log in. Are you wanting to change that for this branch? https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-nominate-for-release/+merge/38733 | 16:37 |
| deryck | abentley, hmmm, my instinct is to say show the link, though I did say don't show it initially. I was thinking of how we do milestones. but that is different, given it's an edit item in the bugtask table. | 16:39 |
| abentley | deryck: thanks. | 16:40 |
| abentley | bdmurray: please change it so that a link is shown for anonymous users. | 16:40 |
| bdmurray | abentley: will do | 16:40 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || Reviewing: rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bdmurray | abentley: okay changes made and pushed | 16:56 |
| abentley | bdmurray: looking. | 16:57 |
| abentley | bdmurray: don't logged-in users have launchpad.View? | 16:59 |
| === jcsackett changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || Reviewing: rockstar || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | jelmer, EdwinGrubbs: mp when you have time is here: https://code.edge.launchpad.net/~jcsackett/launchpad/join-not-allowed-244527/+merge/38949 | 17:11 |
| rockstar | jelmer, did you find my mp? | 17:15 |
| jelmer | rockstar: yep - it's the merge-queues-api one, right? | 17:15 |
| rockstar | jelmer, yup | 17:15 |
| jelmer | EdwinGrubbs: Can you review jcsackett's branch? I will probably not have time for another review after rockstars. | 17:17 |
| EdwinGrubbs | jelmer: I'll do it. | 17:19 |
| === benji is now known as benji-lunch | ||
| === Ursinha-lunch is now known as Ursinha | ||
| === deryck is now known as deryck[lunch] | ||
| bdmurray | abentley: right. okay, fix pushed. I was on a call. | 17:30 |
| abentley | bdmurray: r=me | 17:33 |
| === salgado-lunch is now known as salgado | ||
| salgado | leonardr, do you still need help with that branch or did sinzui's reply clarify things? | 18:01 |
| leonardr | salgado: i'm working on it now, i'll ask you if i have questions | 18:01 |
| salgado | ok, cool | 18:02 |
| === deryck[lunch] is now known as deryck | ||
| === benji-lunch is now known as benji | ||
| === jelmer changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | salgado, any ideas how to word the first sentence so that it mentions "all applications"? would it be enough to put that information in the header? | 18:42 |
| salgado | leonardr, well, the first sentence I suggested already mentions "all applications" | 18:45 |
| leonardr | salgado: that's in the second sentence | 18:45 |
| leonardr | first sentence is "the (whatever) called (whatever) wants access to your launchpad account" | 18:45 |
| bdmurray | EdwinGrubbs: could I get https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-bug-expiry/+merge/38958 added to the queue? | 18:46 |
| EdwinGrubbs | bdmurray: sure | 18:46 |
| salgado | leonardr, sorry if I wasn't clear, but I meant to drop that | 18:46 |
| salgado | I don't think it's of any help | 18:47 |
| leonardr | salgado: now i'm quite confused. you suggest "Would you like to give all applications running | 18:48 |
| leonardr | on <mycomputer> full access (including making changes) to your Launchpad | 18:48 |
| leonardr | account?" | 18:48 |
| leonardr | but now you don't think that's helpful? i think it is | 18:48 |
| leonardr | otherwise it's not clear to a naive user exactly what it means to give "your computer" access, is it something on the kernel or hardware or what | 18:49 |
| salgado | leonardr, I meant that that should be the first sentence | 18:49 |
| leonardr | ah, the current first sentence should be removed | 18:49 |
| salgado | yes | 18:49 |
| leonardr | i respectfully disagree--i think the wording is fine now (although the heading could be changed), and that part of the ui was already reviewed by someone else. how can we resolve this? | 18:50 |
| leonardr | salgado -^ | 18:51 |
| salgado | leonardr, well, I don't think there's much value in reviewing parts (in this case, one or more sentences) of a UI -- they ought to be reviewed as a whole. I suggested you drop it because it doesn't (IMHO) help the user in deciding what to do, but if you disagree that's fine, although it's good to state your rationale in the MP for further reviewers to know | 18:57 |
| leonardr | salgado: ok. my rationale is that "this computer wants access to your launchpad account" is very vague, especially to a naive user | 18:58 |
| leonardr | and even to a knowledgeable user, because it's not technically accurate. your *desktop session* wants access to your launchpad account | 18:59 |
| leonardr | but explaining that will confuse naive users even more | 18:59 |
| salgado | is that why you want to keep the vague first sentence? | 19:02 |
| leonardr | salgado: yes. the vague first sentence makes a naive user say 'oh, ok', and the second sentence makes an experienced user say 'oh, that's what they mean by 'my computer'' | 19:04 |
| salgado | leonardr, I buy that | 19:07 |
| === gary_poster_ is now known as gary_poster | ||
| salgado | leonardr, I can't think of a way to include "all applications" in the first sentence, though, but maybe sinzui or mrevell can? | 19:09 |
| === bdmurray changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jcsackett, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | EdwinGrubbs: https://code.edge.launchpad.net/~lifeless/launchpad/bug-663828/+merge/38967 | 19:46 |
| === lifeless changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: - || queue: [jcsackett, bdmurray, lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | salgado, sinzui, now i have a real ui question | 19:47 |
| leonardr | i'm repeating all the "allow access" buttons as per your suggestion, and grouping them separately from the "deny access" | 19:47 |
| * sinzui channels beuno | 19:48 | |
| leonardr | i need to do two things: 1) indent those buttons with respect to the text they're grouped under | 19:49 |
| leonardr | 2) put some vertical space between those buttons | 19:49 |
| leonardr | i have no idea how to do 1) | 19:49 |
| leonardr | for 2), putting each button in its own paragraph gives the right amount of space, but that seems weird. sinzui's suggestions were to use a <div> (which doesn't add vertical space) or a <br /> (which doesn't add enough) | 19:49 |
| sinzui | I anticipated 2 and wrote the css for 1 | 19:49 |
| leonardr | any ideas? | 19:49 |
| * sinzui looks as CSS | 19:49 | |
| leonardr | sinzui: the only css you have is class='actions', and that's not doing anything for me | 19:50 |
| sinzui | leonardr, the class .subordinate to indent them 2em. we have forms that are generating <ol class="subordinate"> that do extra indentation | 19:52 |
| leonardr | all right | 19:52 |
| sinzui | leonardr, there is a ul.buttons class. That sounds like you want, but i cannot think about where it is used. It ensures padding around them | 19:53 |
| lifeless | EdwinGrubbs: are you still ocr? | 19:53 |
| sinzui | leonardr, so maybe <ul class="buttons subordinate"> and a li around each button | 19:54 |
| EdwinGrubbs | lifeless: yes | 19:55 |
| leonardr | sinzui: class="buttons subordinate" presents all the buttons on one line with no indentation... | 19:55 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: jcsacket || queue: [jbdmurray, lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | EdwinGrubbs: ok cool | 19:56 |
| leonardr | as does "buttons" on its own. so maybe "buttons" is not what we want | 19:56 |
| sinzui | leonardr, sorry, I see the li rule for button is causing that | 19:59 |
| sinzui | leonardr, try subordinate by itself | 20:00 |
| leonardr | sinzui: looks great | 20:04 |
| leonardr | thanks | 20:04 |
| leonardr | sinzui, salgado: one more idea. i could change [allow foo to access my launchpad account] "Permanently" to "Until I Disable It". good idea? | 20:05 |
| sinzui | +1 | 20:05 |
| salgado | +1 | 20:06 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: bdmurray || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | salgado, sinzui, if you refresh the screenshot on my personal site it should be more to your liking now | 20:23 |
| salgado | leonardr, what's the url again? | 20:24 |
| leonardr | salgado: let me just attach it to the bug | 20:24 |
| leonardr | salgado, sinzui: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703638/+files/Screenshot-1.png | 20:26 |
| _mup_ | Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436> | 20:26 |
| leonardr | s/Permanently/Until I Disable it/ | 20:26 |
| sinzui | leonardr, have you seen what the first four buttons look like horizontally? I wonder if allow on one line and disallow on another line is easier to read | 20:29 |
| leonardr | sinzui: i can show it to you and you can decide. i think separate lines work better | 20:30 |
| salgado | yeah, I was thinking about that: [Permanently], [For One Hour], [For One Day], [For One Week]\n or [Do not Allow...] | 20:30 |
| sinzui | leonardr, I need to pickup my children. | 20:30 |
| leonardr | ok | 20:30 |
| leonardr | sinzui, salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703651/+files/Screenshot-2.png | 20:38 |
| _mup_ | Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436> | 20:38 |
| salgado | leonardr, maybe with the commas and the "or" before the [Do not Allow] button, as I suggested? | 20:39 |
| salgado | leonardr, maybe also placing the "accept" buttons on the same line as the sentence? | 20:40 |
| leonardr | salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703661/+files/Screenshot-3.png | 20:42 |
| _mup_ | Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436> | 20:42 |
| leonardr | looks weird to me | 20:42 |
| salgado | leonardr, indeed. can you try just on the same line as the sentence, without commas? | 20:45 |
| leonardr | salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703663/+files/Screenshot-4.png | 20:49 |
| _mup_ | Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436> | 20:49 |
| salgado | leonardr, I think that looks nicer; just need some more vertical space between the buttons | 20:51 |
| leonardr | salgado: how can i do that? even <br> isn't helping | 20:52 |
| leonardr | ok, got it | 20:54 |
| leonardr | however, you should know that the "For One Week" button is wrapping around to the next line | 20:55 |
| leonardr | is that ok with you? | 20:55 |
| salgado | oh, in that case it's probably better to leave them all on a separate line | 20:55 |
| salgado | is it wrapping after your change? maybe we can do something else that adds the v spacing but don't cause the wrapping | 20:56 |
| leonardr | salgado: https://bugs.edge.launchpad.net/launchpad-foundations/+bug/663436/+attachment/1703675/+files/Screenshot-5.png | 20:58 |
| _mup_ | Bug #663436: Allow temporary integration of desktops into the Launchpad web service <Launchpad Foundations:Triaged> <https://launchpad.net/bugs/663436> | 20:58 |
| leonardr | salgado: even if we do something fancy, a long computer name like "Salgado's Computer" might make it wrap | 20:58 |
| salgado | good point; it doesn't even need to be that long | 20:59 |
| salgado | I like the -5 screenshot best, I think | 20:59 |
| leonardr | ok | 20:59 |
| === matsubara is now known as matsubara-afk | ||
| leonardr | i will run that through ec2 and land tomorrow | 21:00 |
| salgado | leonardr, cool, but I'm still not a graduated UI reviewer, so sinzui must give another thumbs up | 21:07 |
| leonardr | ok, put a comment in the review and ask sinzui to weigh in when he comes back | 21:08 |
| sinzui | leonardr, salgadoI like ...-5 too | 21:09 |
| sinzui | leonardr, If I was mocking this page in text I would have typed... | 21:09 |
| leonardr | sinzui: i pushed my revisions to achieve screenshot #5 | 21:12 |
| sinzui | leonardr, http://pastebin.ubuntu.com/517028/ | 21:12 |
| sinzui | leonardr, I would have used "or" to introduce the other kind of decision | 21:12 |
| bdmurray | EdwinGrubbs: the configure-bugtracker link does appear on launchpad.net | 21:12 |
| bdmurray | EdwinGrubbs: in the "Configuration Progress" portlet area - https://staging.launchpad.net/bughelper | 21:13 |
| salgado | sinzui, as in http://launchpadlibrarian.net/57938554/Screenshot-3.png? | 21:14 |
| sinzui | leonardr, also, check capitalisation. Button are title case so i think "It" and "Not" are preferred | 21:14 |
| sinzui | salgado, I see. I do think or on the same line as the button looks odd | 21:14 |
| sinzui | Trying to make the options into a sentence look odd in itself | 21:15 |
| sinzui | 5 is definitely easier to read than 3 | 21:16 |
| leonardr | sinzui: "it" is an article. i think https://dev.launchpad.net/UserInterfaceWording is wrong in saying you have to capitalize the last word (i think it means the first word?) but i'd just as soon capitalize everything | 21:17 |
| leonardr | but /me is far beyond the point of caring and will do whatever will get this thing approved | 21:18 |
| sinzui | leonardr, I think it is a pronoun | 21:20 |
| leonardr | yeah, you're right | 21:21 |
| leonardr | sinzui, salgado: http://launchpadlibrarian.net/57940682/Screenshot-6.png | 21:21 |
| salgado | looks good to me | 21:22 |
| leonardr | actually, /me no longer clear why "Allow foo to Access my Launchpad Account" is the case it is | 21:22 |
| sinzui | I like 6 best, but I do not want to fight anyone over it | 21:22 |
| salgado | leonardr, it shouldn't | 21:22 |
| salgado | that's from when it was a button | 21:23 |
| salgado | "Allos foo to access my Launchpad account:" | 21:23 |
| sinzui | salgado, ? 'Until I Disable "foo"' | 21:24 |
| salgado | sinzui, the capitalization of the sentence preceding the buttons is wrong. it should be "Allow foo to access my Launchpad account:" | 21:25 |
| sinzui | okay | 21:25 |
| sinzui | salgado, leonardr which layout do you like best, 5 or 6 | 21:27 |
| salgado | I prefer 6 | 21:27 |
| leonardr | either is fine w/me | 21:27 |
| EdwinGrubbs | bdmurray: but the configure-bugtracker link only shows up on the lp.net/$project page if the user has launchpad.Edit permission. It does not show up if the user has launchpad.BugSupervisor permission. | 21:28 |
| sinzui | leonardr, salgado, okay 6 is the layout. Thanks you both for taking time to do this extra work. | 21:31 |
| bdmurray | EdwinGrubbs: ah, okay then | 21:31 |
| leonardr | sinzui, salgado, i have pushed final version | 21:33 |
| sinzui | leonardr, I approved your branch. land away | 21:38 |
| leonardr | ok | 21:38 |
| leonardr | thanks | 21:38 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || Reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha is now known as Ursinha-afk | ||
| jcsackett | back | 22:14 |
| lifeless | EdwinGrubbs: so that branch is entirely mechanical | 22:38 |
| lifeless | EdwinGrubbs: I'd hope you don't need to spend much time on it | 22:39 |
| EdwinGrubbs | lifeless: I just started on it a little while ago. | 22:39 |
| lifeless | kk | 22:44 |
| === Ursinha-afk is now known as Ursinha | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
