On May 19, 2011, at 6:35 AM, Danilo Šegan wrote: > Hi Gary, > > У сре, 18. 05 2011. у 17:48 -0400, Gary Poster пише: > >> I'm cc'ing everyone else so they can see where we are. Everyone else, >> if you have some spare cycles to help out, check in with Danilo or me >> to see if we can give you a branch to do. >> >> I figured out what was going on with the mute UI, and did a spike to >> get it to work experimentally. It turns out there were a number of >> changes that needed to be made. >> >> The branch is lp:~gary/launchpad/bug-772763-remove-unmute-dialog > > I looked at your branch a bit. I find it weird that you modify unmute() > call to return the previous direct subscription considering that one is > already available in the page JS in my tests (eg. with fresh db-devel, > try subscribing at a level different than default, muting that > subscription, refreshing the page [to confirm the data is available from > the get-go], then pop up the 'subscribe' dialog and notice how the > 'unmute and update my subscription' shows the correct bug notification > level).
The subscribe dialog is drawn by the server though, not JS, *after* you click the "subscribe" or "edit subscription" links. So the information that the overlay has is not inherently available to the unmute code. Or am I missing something? > > IOW, server-side code already loads the sufficient information into the > page. Not saying it's pretty, but just saying it's there already. :) I didn't see it available to the unmute JS, but I may have simply missed it. Is it in the LP.cache and updated there? I didn't see sufficient code for that to happen either, but that stuff seems to work in mysterious ways. Let me know what you had in mind. > >> We have a mostly working spike (I was clicking around on >> ttps://bugs.launchpad.dev/firefox/+bug/1, fwiw). I think we now need >> to redo it with tests, dividing it up into smaller branches. > > Ideally, we'd move all the mute stuff into a separate module (at least > imho). Eg. lp/bugs/javascript/bug_muting.js. I'm not really interested in that myself. It's, what, three functions now? And in a module that we'll need to significantly refactor for the next bug. I won't stop you, but I'm not excited to do it myself. > > All of the Subscription class use for bug muting seems very much > unnecessary imho, but I didn't dare change it previously. So...you are glad that I ripped it out? Or concerned? Or both? :-) > > Your branch also changed handle_advanced_subscription_layer which I > didn't expect you to touch (i.e. since you wouldn't be popping up a > dialog, you wouldn't need to change it). I think that brings up one big > issue with the bugtask_index_portlets.js as it is: one always wants to > go and clean it up some more :) heh. The reason I touched the dialog is twofold. First, I decided that having the "subscribe" link while you were muted but had a hidden description was going to be helpful for some people in some situations (notably, if you have forgotten that you muted, and are just looking for a "subscribe" link, this will tell you what's going on). Second, keeping that functionality made the branch smaller, from a "get this bug done" perspective. > >> In particular, there are some server changes that need to be made. I >> think they can be tested simply. These could be one or more server >> branches. I wonder if the queries (in particular the also notified >> code) need to be made more performant. Then again, they might need to >> be rethought more deeply for bug 772754. > >> Then we need a separate JS branch. I'll probably try to do a very >> basic, minimal JS test, since we are probably going to trash so much of >> this JS code for bug 772754. >> >> If you get done with your other task and want to take some of this >> tomorrow morning, feel free. Maybe a first step would be to get my >> branch running and see if you agree with some of the UI decisions I >> made--or at least don't strongly disagree, since we are on a >> timetable. I'll jump in when I get going, though tomorrow is my call >> day, so my availability will be somewhat limited. > > I am trying to make sure that my in-progress branch and your in-progress > branch make sense. I'll probably merge them together because of all the > refactorings you did. If you mean a full merge, I'm a bit concerned by that. As I said, I was hoping we could land smaller branches, and I regarded what I did as a spike. Maybe you merge the two just to verify that we are on the right track, and then we use that to determine what next branches we actually need? Gary -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
