So, I just started my day off with a big bowl of Apple Cinnamon Cheerios and some fresh orange juice, c/o Mozilla Messaging. Thanks, team. 😀
Another great thing about Mozilla is that, since its spread out around the world, somebody is awake and working pretty much 24 hours a day. That means if I write a bunch of code and go to bed, there’s a chance that when I wake up the next morning, the code review is done and I have some feedback on my next steps.
And that’s more or less what’s happened at the start of today. That add-ons manager grouping feature I was working on for Firefox got looked at in the night, and I got some feedback on some changes I can make. Awesome.
So here’s the scoop:
First off, a conversation got started regarding where add-ons with pending installs or uninstalls go. The answer: pending installs should go into the group they’ll be in once the install is complete, and pending uninstalls should stay in the group that they were in when the install happened (in order to prevent the add-on from jumping around in the list). That’s good – that makes my job easier, I think.
With regards to implementation, I don’t think my nifty Shwarzian Transform is gonna fly:
I don’t think that this is the right approach to take here. Instead the more straightforward way is to just make sortElements accept an array for aSortBy (and update all callers to pass one) of fields to sort by in order of preference. If the elements match by the first field then move onto the second and so on. For the non-search list views use a specially named field “uiState” and then use your function as the comparison function for that field.
So this approach involves me changing a bit more code. See, my original approach was to try to change as little as possible. I guess that’s just me being the “new guy”, and trying not to rock the boat. But clearly, they want me to go deeper. I’m happy to oblige!
But there’s a bit more complication. According to the Bugzilla page, this bug I’m working on depends on this other bug, where somebody else is also tinkering with the sorting functions. That means I have to be uuber careful, and make sure to base my work off of their patches.
In particular, it looks like this patch is going to be altering the sorting tests, and removing the ability for Firefox users to sort add-ons by anything besides the add-on name.
So what I’m going to do is download and apply this patch, and then start basing my work off of it. Each time the patch is updated in Bug 623207, I’ll just re-base my work off of it. Nice.
Ok, so first of all, I wipe out my old work using hg strip (using Blake’s handy script to find the HEAD revision number of that branch). Next, I grab the patch for 623207, and use patch to apply it. Then, I use pnew to create a new pbranch with that change, and then create another pbranch on top of that. That second pbranch is where I’ll do my work. If/when the patch to 623207 gets updated, I’ll update the first pbranch, and then merge it into the second. Awesome-sauce.
Argh. It seems I have to recompile in order to get that 623207 patch to work. And not an incremental compile either, since “make” doesn’t seem to do much in toolkit/mozapps/extensions. *sigh*…compiling…
Just stopped by UofT to drop off my old keys, and get my old computer wiped. Stopped by and talked to Karen, and helped two new MarkUs students. Kind of bitter sweet moment. Goodbye school. It was a long battle. Well played.
I’ve finished a draft of my add-on grouper/stratifier. Now I’m going to check out these tests. Without my change, the tests in browser_sorting.js all pass. With my patch, 3 fail. Ok, so I think I’ve found the right tests. Lets see whats up…
Ok, it looks like the tests were trying to call my new sorter, and didn’t know that it had to pass an Array instead of a string. Fixed that, and all tests are passing. Sweet.
Now let me try all of the extension tests…ok, without my patch, 10 fail. With my patch… OH SHIT. 194 failures. That’s a big deal.
Ok, I think I’ve fixed those tests. But now when I run the extension tests, one of the tests seems to take forever…what’s the deal? Turns out, this test does this periodically, even without my patch. Hrm. So I guess I don’t have to worry about it.
After a few runs, I’ve got the same number of passing tests as there were before my patch: only 10 fail. Nice.
So now I have to try to write some new tests.
Suite! Tests written.
It takes me a while to run the Firefox Mochitests on my machine. If I were to do them all, it’d probably take at least an hour. Running the add-ons manager tests takes about 5 minutes. And in either case, I can’t use my machine, because Mochitest needs me to keep focus on Firefox while its running the tests. Basically, this means if I want to run the tests, I give up my machine, and go snack on something in the kitchen. That was cool at first, but after a while, giving up my machine for 5 minutes seemed pretty lame.
So, luckily, there’s a machine in the office called TheFlash, and, as its name suggests, it’s SUPER fast. Like, lightning speed. Compiling Thunderbird from scratch? 7 minutes flat. Jeebus. Anyhow, Blake got me an account on TheFlash, and I’ve pushed my changes to a Firefox instance over there. Firefox is compiling, and then I’ll try out my tests over there. Awesome.
Tests pass! Lovely. And Blake just took a look at my code and showed me a neat trick:
So I’ve got an Array of uiState’s, like so:
const UISTATE_ORDER = ["enabled", "incompatible", "disabled", "blocked"]
And I wanted to sort a collection of these values in order that they appear in that Array. So something like:
["disabled", "disabled", "blocked", "incompatible", "disabled", "enabled", "enabled", "blocked", "disabled"] # Would become ["enabled", "enabled", "incompatible", "disabled", "disabled", "disabled", "disabled", "blocked", "blocked"]
(this sort of thing is useful if each of those original entries is associated with something like, I don’t know, a Firefox add-on…)
In Javascript, we use a sort command, and we pass a function to do comparisons. Normally, I’d do something like:
function uiStateCompare(a, b) {
if(UISTATE_ORDER.indexOf(a) < UISTATE_ORDER.indexOf(b))
return -1;
if(UISTATE_ORDER.indexOf(a) > UISTATE_ORDER.indexOf(b))
return 1;
return 0;
}
But there’s a more concise way to say this:
function uiStateCompare(a, b) {
return (UISTATE_ORDER.indexOf(a) - UISTATE_ORDER.indexOf(b));
}
Which makes total sense. If UISTATE_ORDER.indexOf(a) < UISTATE_ORDER.indexOf(b), then of course the difference is less than 1. Anyhow, I thought this was a pretty neat trick. Thanks Blake.
Alright, patch is scrubbed and ready for posting on Bugzilla….here goes!
Ok, patch posted. Home time.