Implements groups (issue #72)

[igorofc]

• Adds a new "Add group" button in the settings page;
• From the popup that is created, the user can select the engines that will belong to the group;
• Clicking outside the popup or pressing the Esc key closes the popup. Pressing Enter is the same as clicking the save button.
• The first engine that is selected will be the main one. This means it is the one that will be first created (and active for some behaviours);
• The group engines can be ordered by moving the dragger element.
• To set a new engine as main, just drag it to the top of the list.
• By default, the group icon is a circle for which the user can choose a different color by clicking on it.

@CanisLupus, I noticed that the code inside onContextMenuItemClicked was very similar to the one in onSearchEngineClick, so after some modifications I'm just calling this latter function from the former. Maybe this is a first step to unify them or if there are reasons to keep them separated we could roll back the changes.

Initially I had created the Edit button in the expandable area, but later I found it better to just click on the searchUrl field to edit the group.

I renamed searchUsingSearchApi to search because it will also be used for the custom engines to search the old way.

I'm considering it done, but since there were many changes it's very likely I'm missing something. Let's review it :)

[CanisLupus]

Hey Igor, thank you for the hard work on this! :) For now I'm just letting you know that I'm currently away for a few days. I'll be sure to let you know when I review it. Cheers!

[CanisLupus]

Hi! I've checked all(?) added user functionality but I haven't seen the code changes yet. :) This was so I can get a better understanding going into it later.

While doing searches I found no problems at all. It seems solid, searches using the selected behaviours, it does the sensible thing when selecting "Open in this tab", works with nested groups. All great! I'll do an even more detailed test when merging the PR and/or before releasing to the public, but it seems golden.

The group creation/editing popup also looks great, like I said before. The popup opening by clicking the engine names in the group is (I think) clear enough to the user (hovering changes the cursor, which helps).

I'd like to suggest a few small changes/fixes, if you agree. They are mostly minor things related to the popup usage, some of them purely visual. Let's see:

1. Fix engine hover highlight outside popup to the right (probably fixable by adding margin right equal to margin left, and perhaps removing width 100% if that doesn't break other things, that is).

2. (probably ridiculous on my part to worry about this, but the fix seems simple) Very short window heights cause the popup elements to overflow it. Hidden vertical overflow or adding a min height to the popup should work!

3. It was not immediately obvious how to remove an engine added to a group (which you can do by clicking them). A possible solution is to add a permanent text saying "Click an engine to remove it." below the added engines (a "close" button that appears all the way to the right of the engine on hover would also work, but it's significantly more work).

4. Deleted engines should probably return to original order. Right now a removed engine goes to the top of the list. I'm not sure if this is intended but it is a bit disorienting. ;)

5. Pre-fill group name when editing the group. Right now it does pre-fill it but as the placeholder name, not the actual text in the input field. It would make it easier to edit. Although I'm aware that you can edit it from the engines table too, if the user has the popup open it should be easy to edit. ;)

   Finally, the color picker:

6. If we replace the group data image with a URL, it should still be possible to pick a color again in the group editor (right now it becomes unclickable). It could be clickable and start updating when we select any color in the picker. If we cancel the colors dialog, it returns to the current icon.

7. When clicking to open it selects Hue 160 with RGB 0,0,0 but it would be better to open with the current color, like for example the options "Popup highlight color" does.

8. Clicking, selecting any color and then cancelling currently causes the color to go to black.

9. Changing color seems to draw the new circle over the old one (or something like that :p), causing artifacts. Example: Original (perfect): After picking "white" you can see the old color behind/around it: Picking the exact same color again, you can see the circle is aliased now, compared to the original:

   Would it be possible to redraw the circle from a clean state?

Please let me know what you think about these and if you are available to make changes (if you agree, of course). If you aren't I can take a look later too!

Cheers!

[igorofc]

Hey, it's great that you liked it.

Just some things:

It was not immediately obvious how to remove an engine added to a group (which you can do by clicking them). A possible solution is to add a permanent text saying "Click an engine to remove it." below the added engines (a "close" button that appears all the way to the right of the engine on hover would also work, but it's significantly more work).

I like the second idea better. I think we could show the button but keep removing the engine from the group when clicking on the row (or should it only be removed if clicking on the button?)

If we replace the group data image with a URL, it should still be possible to pick a color again in the group editor (right now it becomes unclickable). It could be clickable and start updating when we select any color in the picker. If we cancel the colors dialog, it returns to the current icon.

You mean it should replace the new icon back to the circle when we update the colors?

Thanks for pointing these details out. I'll definitely try to fix them.

[CanisLupus]

I like the second idea better. I think we could show the button but keep removing the engine from the group when clicking on the row (or should it only be removed if clicking on the button?)

I think that if I saw a delete button as a user I'd assume that clicking the engine itself was safe. I would probably expect it to either do nothing or to allow dragging just like the drag icon. :) That said, I found the current implementation to be easy-to-use, just not very understandable at first.

You mean it should replace the new icon back to the circle when we update the colors?

Not sure if I understood, but trying to clarify: What I meant is that a group icon with a "custom icon URL" should still be clickable. In that case it would open the picker and transform into a colored circle (random color?), updating as normal as you choose colors. But if you cancel it should return to the original custom icon URL.

Original:

Click to pick a color:

Then cancel (no OK):

Thanks for pointing these details out. I'll definitely try to fix them.

Thanks, it's much appreciated! :D

[igorofc]

I think that if I saw a delete button as a user I'd assume that clicking the engine itself was safe. I would probably expect it to either do nothing or to allow dragging just like the drag icon. :) That said, I found the current implementation to be easy-to-use, just not very understandable at first.

Got it! So only remove the engine when clicking on the remove button then. That sounds good. Something you just taught me is to see things from the perspective of the end user.

Not sure if I understood, but trying to clarify: What I meant is that a group icon with a "custom icon URL" should still be clickable. In that case it would open the picker and transform into a colored circle (random color?), updating as normal as you choose colors. But if you cancel it should return to the original custom icon URL.

I meant exactly that :). I'll do it.

[igorofc]

Not sure if I understood, but trying to clarify: What I meant is that a group icon with a "custom icon URL" should still be clickable. In that case it would open the picker and transform into a colored circle (random color?), updating as normal as you choose colors. But if you cancel it should return to the original custom icon URL.

@CanisLupus, I got stuck trying to solve this. Right now, I've set it so that when the custom icon is clicked, it's replaced by the randomly colored circle as expected. The problem is I couldn't find a way to handle when the user cancels the color picker (or closes it's window). How do I know when to restore the previous icon in that case?

[CanisLupus]

Ah, I see. I thought we could get different events for OK and Cancel, but probably not, or at least not easily. That must be why I also don't return the color pickers to the old value on Cancel (I did it a loooong time ago).

So no problem. :) Replacing by the colored circle on click is basically solving the issue already. I can take a look later but I may not have luck either.

[igorofc]

Hi Daniel, I've tried a workaround but it comes with a cost as always. It seems that changing the value property of the color picker after it opens up makes the change event fire when it is closed or cancelled, but doesn't actually change the value. So I used a setTimeout to change that property after clicking on the modified icon. So now we know when the color picker is closed.

Since the change event also fires when selecting a color, we have to have a way to tell if the user cancelled or chose a color. What I did was set the color picker value to the current background color (of the settings page) before opening it up. The logic is that in the onchange function we compare the new value with the background color. If they match it means the user cancelled the color picker.

So the cost is that if the user happen to like the random color that was applied to the circle and click Select on the color picker without trying a new color, the custom icon will not be replaced because the value of the color picker remained the same as the background color. The same thing would also happen if the user does try a new color but selects the same color as the background although this would be very unusual and less likely to happen.

To summarize, it works if we assume the user changes the color of the circle and doesn't select the same color as the background of the page.

When you have a look you'll understand better. I did all the other fixes we discussed. Let me know if there's anything lacking.

Have a good day!

[igorofc]

Hey @CanisLupus, I was thinking and a better alternative would be to just revert to the default circle icon (with the default color) when clicking on the custom one inside the popup, without opening the color picker. We could also ask for confirmation for that. What do you say?

[CanisLupus]

Hi Igor, it may take a while for me to get back to you on this PR, but I'll try to review a bit of code every day.

After seeing from your description how difficult and possibly how error-prone this was to implement, I agree with your suggestion. ;) This was just a detail to solve the problem of not being able to return to a colored icon after setting a URL. Resetting it on click with a confirmation question seems perfect and doesn't need timeouts, so I'm all for it.

I can implement this while I review things (or when I finish). :) I'll make some commits while reviewing, so it's probably best if there are no further changes from your side since I can't modify your PR directly without, I think, making another PR to your PR branch. Sigh... xD

[igorofc]

Hey @CanisLupus, I went ahead and implemented it hoping you'd agree with me on that issue. So I'm pushing the changes and will be making no more :). I'll look forward to helping with anything.

Cheers!

[CanisLupus]

Haha awesome, thank you! 🙏

[CanisLupus]

Hey Igor :)

I'm still in the early parts of the code review, but I wanted to ask you something. For OpenResultBehaviour.NewWindow you do this:

browser.windows.onCreated.addListener(windowOnCreated);
browser.windows.create(options);

However, if we do it similarly to the other behaviours:

browser.windows.create(options).then(windowOnCreated);

...we also seem to get the event at the right time, and it already comes with a tab argument (instead of window), which avoids calling browser.tabs.query in windowOnCreated.

Since I don't trust WebExtensions and JavaScript too much, I have to ask: did you see something that didn't work when using the second approach that led you to use windows.onCreated? Or was it simply a case of "trying approach 2 didn't cross my mind"? :) Just making sure because it may very well be like this for a reason.

Cheers! Daniel

[igorofc]

Hi Daniel. It was more like that second option there. :) So this is where that pesky tabs property is. I never tried that approach. It's great it works 'cause we eliminate the need for that listener.

[CanisLupus]

I don't know what I was seeing yesterday anymore, but today I quickly realized that windows.create returns a promise for a window, not a tab (makes sense), so although it still saves us from registering and unregistering the event, we still need to get tabs[0].

However, here "tabs" seems to be present, both on Firefox 63 and 81.0.1. I wanted to use it but you made it clear that it wasn't working on your end, so do you have any idea why? Mozilla says it's been implemented since Firefox 45, but there could have been a bug in some version that you were using.

(To be clear, I changed the declaration to function windowOnCreated(window: browser.windows.Window), so that the typechecker knows that window is a Window and I can avoid errors.)

[CanisLupus]

The TypeScript definitions declare tabs as tabs?: tabs.Tab[];, meaning they could be null/undefined, but the Mozilla docs don't mention any of this (would be useful to know if there are any situations where it is null/undefined).

[igorofc]

However, here "tabs" seems to be present, both on Firefox 63 and 81.0.1. I wanted to use it but you made it clear that it wasn't working on your end, so do you have any idea why? Mozilla says it's been implemented since Firefox 45, but there could have been a bug in some version that you were using.

I can't see the tabs property when using the event listener like I was doing (it is the only property missing). But if I use the approach you wrote, it is there. I can't tell why that is. Can you see it in both situations over there?

[CanisLupus]

Ah, you're right, it's undefined only in onCreated. Thanks, that's a mystery solved then. It's possible that Firefox triggers the window creation event before it deals with any tabs that should go along with it, so they're undefined. The Promise for windows.create(options) only finishes later, so it would have the tabs.

I'm also assuming this happens in a very tiny interval of time, so browser.tabs.query ends up working too as it takes more time. All just a guess, though. The docs don't have any mention of this for onCreated either.

[CanisLupus]

Review completed for the hardest part, which is swift-selection-search.ts! :) I refactored quite a bit and I tend to write a lot sometimes, so... :p I hope you'll agree with me on the refactorings.

4 commits for now: https://github.com/CanisLupus/swift-selection-search/commit/8c9be807307851b9a1800ec7b7ce45f513391a21 https://github.com/CanisLupus/swift-selection-search/commit/a3e2369cd7011ed9fe181629681a69ce09851b8e https://github.com/CanisLupus/swift-selection-search/commit/e11a158f06c9b47b9a53e4122a30893a82a87c3b https://github.com/CanisLupus/swift-selection-search/commit/c1a10aff6c44bc5feef88d80bb984ee70783184c

My general process is/was:

1. Make formatting changes where I think it needs them, without looking at the code contents.
2. Understand and refactor code in swift-selection-search.ts.
3. Understand and refactor code in settings.ts + html, page-script.ts (still haven't done this).

Refactorings in swift-selection-search.ts

I had some trouble understanding the changes in swift-selection-search.ts due to a few things, so I knew that the code was going to need some deeper changes, but I still started with the formatting changes and then the smaller refactorings that you can see in commit a3e2369c, which also helped me get a better grasp of everything. The commit should hopefully explain those changes.

Then, regarding the deeper changes: while the idea of recursion in onSearchEngineClick is fine in itself, the flow of "onSearchEngineClick" calling "search", which in turn could call "onSearchEngineClick" again when we're creating windows, all the while having "onSearchEngineClick" check the behaviour and "break" to skip creating tabs is...a bit much to understand cleanly. ;) Your comments were very helpful in all places, though.

The code also did a few specific things that complicated the flow, such as:

• reversing the engines except the first one.
• jumping indexes by either 1 or 2 in search but only for certain behaviours.
• changing the old method openUrl to support both SearchEngine_BrowserSearchApi and SearchEngine_Custom, which required some extra arguments that were only used for one case or the other.
• casting "engine_SSS as any" to pass to search, which is a bit...sketchy :p
• things like "tab.index + jump || lastTabIndex + 1", which are hard to understand. Is the OR because the first part can be undefined/null? In that case we can use "??" instead, although it's less well-known. Or could it be 0? Is it jump or tab.index that can be undefined? Etc.

I understood why all those were done, though. And I appreciate the fact that you essentially implemented all opening behaviours for internal browser engines while doing this! However, I was confident this could be improved upon. :)

My thoughts starting were:

1. We want to go through the group engines in the simplest way possible (a for-cycle, if we can) and search using each engine.
2. To avoid callback-hell and the current 2-method recursion (which has multiple execution paths and a few "gotchas"), we could probably just WAIT for the completion of an engine's tab creation (which is almost instantaneous), then do stuff for the next engine, and so on.
3. Simplifying "search" would be cool.

Essentially:

• I was not sure about 1, but 2's ideal solution is chaining Promises using then().

• Since what we want is always to create a new tab/window (or use the current one) and then open a URL or conduct a browser search in it, we could have a method just for the "open a new tab" part, and it could return a Promise on which we could call then() to setup the next part i.e. the actual search. The first part is the same no matter the engine (custom or browser), which is great.

  Conclusion: "search" was transformed into "getTabForSearch" and it returns a Promise that we can wait for. This solves point 3, although the other code that was in "search" still had to go somewhere, which would be onSearchEngineClick.

• While I was chaining the Promises for all the group engines I had that bad feeling that I was creating weird code. I use C# a lot and this seemed like something that C# could solve with async/await. So I searched and I found out that TypeScript supports the async/await keywords just like C# (thanks, Microsoft!), which made this A LOT simpler to do. Now we can have the simple for-cycle (provided we expand the group engines beforehand), except that the code "await"s the completion of each Promise before continuing to the next line. Really, I was so happy that TypeScript has this as well.

• Now it was just a matter of calling the right things in onSearchEngineClick and changing the opening behaviour after the first engine in the group, while increasing an "tab index offset" variable as we go, to achieve the correct behaviour again.

Fixes

I noticed two small regressions, which I fixed. But for both of them you'd have a really hard time noticing problems because they do "nothing" :p. Allow me to explain:

1. I added back options["openerTabId"]. This is useless for SSS itself, but some tab manager addons rely on "openerTabId" to know which tab is the parent of which tabs.

2. Joining the code for onContextMenuItemClicked and onSearchEngineClick was the right choice, but something was lost. SSS has a (deactivated) feature to search for a link's text if the user right clicks a link and uses the context menu. If you want to know the dumb reason it's deactivated you can read more in the comment below line contexts: ["selection"/* , "link" */], in the background script. Anyway, for the copy function, copying any selected text is prioritized over copying a link (it really shouldn't, but it's like that because of the %s thing you read in my explanation) but you switched the if-else around. I fixed it, but it's not a big deal.

Another fix that was not a regression:

• The code before the refactorings had a very edge-casey bug where opening a group as "tab next to current" could make the "main engine" open LAST if the group had another group inside, which ALSO had yet another group inside it. I found that almost by accident and it could be hard to find what was causing it exactly, but the new approach fixes that.

Future changes/fixes (soon™)

I want to change class SearchEngine_Group to extend SearchEngine instead of SearchEngine_Custom. A group doesn't have most of the fields of a SearchEngine_Custom and we can't really say it IS one. It's better to just add the name and iconUrl directly to it. I didn't do it yet since it might affect the other scripts, which I haven't looked at yet.

List of issues I know of and will try to fix:

• Fix discardOnOpen kind of breaking the opening tab indexes for tabs coming after it (I think your code didn't have this problem, but my refactor does)
• Fix clicking engine in popup after adding it to the group causing it to silently be added again and again and again :p (I found this by accident too, it was interesting :p)
• Fix cyclic references (group 1 contains group 2 and group 2 contains group 1), which completely break the settings object when saving (and if it didn't it would cause an infinite loop when searching anyway).
• This is a complicated one. Change settings saving to not clone engines inside groups or at least transform them back into simple references when reading. This causes problems when reading groups from an exported file or from Firefox Sync, since the engines inside the groups will be clones of the original objects, meaning that any changes to an engine will not be seen by a group that contains that engine (name changes, url changes, etc).
• Increase group engines popup width in narrow windows (higher %, probably add max width).
• Fix long engine names being centered in their name box (better seen in narrow popups).

[igorofc]

Daniel, I always appreciate the way you explain the changes you made and why you made them. Your constructive criticism means a lot and motivates me (and certainly future contributors) going forward.

Your implementation of promises will probably fix some odd issue I was having with the search not happening after the tab was created. I wish the code hadn't come out with so many complicated parts and missing bits. I'm certain, though, that your refactoring and fixes will clear things up. It's great you came across those hidden bugs and tested some edge cases. They went unnoticed by me.

I'll update the code asap to see your work - and learn from it. I feel like there are a lot more fixing to be done in the other parts of this implementation so bear with me :pray:. Contributing to this project has been great. I'm available to help with what I can.

[CanisLupus]

Daniel, I always appreciate the way you explain the changes you made and why you made them. Your constructive criticism means a lot and motivates me (and certainly future contributors) going forward.

You're welcome, I'm happy to give feedback. It's great that it motivates you, because the last thing I want is for you to feel discouraged by my reviews. ;)

Your implementation of promises will probably fix some odd issue I was having with the search not happening after the tab was created.

I think discardOnOpen had a similar problem which we'll hopefully be able to fix or mitigate in this next release. It was reported recently in the end posts of #189 (in portuguese! ;)) but I couldn't reproduce it at the time. Sadly I couldn't get it to work even with promises (at least not yet), because Firefox seems to take a few instants to process the URL and if we close the tab as soon as it opens it may simply not do the search (I added a timeout for now, but I don't like that a lot).

I wish the code hadn't come out with so many complicated parts and missing bits. I'm certain, though, that your refactoring and fixes will clear things up. It's great you came across those hidden bugs and tested some edge cases. They went unnoticed by me.

No worries! I should normally be able to see more edge cases since I'm more familiar with the settings and code base. I hadn't noticed them when I first tested, though, but bugs almost always come up (sometimes by pure accident). :)

I'll update the code asap to see your work - and learn from it. I feel like there are a lot more fixing to be done in the other parts of this implementation so bear with me 🙏. Contributing to this project has been great. I'm available to help with what I can.

Thanks! I haven't looked at the rest yet, but I'll let you know.

Cheers!

[CanisLupus]

Just letting you know that although it's been a month I've been sloooowly commiting changes to this. I've adopted a "change things as I go" style for the settings.ts and settings.html file (instead of a few large monolithic commits), otherwise I'd take even longer. 😅 The problem with that is that not all commits are great examples of changes. At least one refactoring broke something that I only found much later (I changed [...editGroup.groupEngines], dumb oversight), and I went back and forth on some decisions.

I've tried to simplify things as much as possible, make some adjustments, and fix the bugs I mentioned before and a few more I found later (still 2 left to go). Some things I changed have nothing to do with being wrong or right, just what felt was better. I've learned to use the HTML template tag along the way and may use it for the engines table later, so you'll see less code creating elements, but some more calls to querySelector. ;)

The changes are readable in the commits so I'll not talk about everything here, but some points:

• I wouldn't use .map to replace a for-cycle, like in [...document.querySelectorAll(".group-popup-container, .group-background-div")].map(node => node.remove()). :p Maybe this was just an oversight. map is supposed to return values, so you could use forEach instead, but personally I just do a for-of cycle instead and avoid the extra list being created by [...]. In another place I also changed your .map and .reduce to a .map and .join (with a small adjustment) as it's simpler and more efficient.
• Although I get that you tried to reuse buildSearchEngineRow to create the elements in the popup, the code ended up creating 3 entire rows for each engine and just using the dragger, icon and remove button. It created everything, registered events, and then only a few things were used, so I made some parts of buildSearchEngineRow reusable in different methods and called those instead.
• Randomly generated colors are now hexadecimal strings instead of rgba(r,g,b). It allows passing it directly to the picker without getting a black color due to the unexpected format. (I went back and forth a lot on color-related code.)
• You had setTimeout(() => groupTitleField.focus(), 100);. I haven't seen the timer being necessary, but I may be missing something. Do you know why this was needed?

Notable user-facing changes, besides the fixes I wrote here earlier (of which there are 2 left):

• Save button is now always disabled when there are no selected engines (it also has no hover effect or special cursor when disabled).
• An empty group is not automatically deleted when its last engine is.
• Popup sizes, centering, etc were improved.
• "iconModified" was removed, so a user defined "data:image/png" icon won't trigger the question to revert to original colored icon. This should be rare but there's still the Cancel button to cancel the change.
• Default group name is simply "Group" and placeholder for name is always present if nothing is written.
• Fixed dialog when adding too many engines to group (couldn't select more engines below the list if too big).
• Fixed ordering of engines after removing (i.e. placing it back in the available list) sometimes being wrong.
• Fixed empty groups being deleted causing a wrong engine to be deleted if the deleted group(s) came before the engine in the list.
• Fixed user-select not working for Firefox 63 (min supported version by SSS). Apparently it doesn't support user-select, so I added -moz-user-select as well (forgot to say that in the commit).
• Fixed changing group name in table not changing it in the description of groups that contain it (unless we refresh).

So now I'll try to fix the final issues so we can get this feature ready soon. :) Please let me know if you want more details on any of the changes.

[igorofc]

Hi Daniel, nice to hear you're working on this. I know it's a lot of work for too little time and figuring out all that needs correcting must be a very strenuous job (so much for my amateurish coding skills). :)

You had setTimeout(() => groupTitleField.focus(), 100);. I haven't seen the timer being necessary, but I may be missing something. Do you know why this was needed?

Well, now that you mentioned it, I tested without the setTimeout and it focus just right. I most certainly had trouble with it in some other project and put it there without testing beforehand. No need for it then.

I'll pull the changes and check it out.

Thanks for the effort!

[igorofc]

I don't know if you're aware but the popup didn't show up here. When I click 'Add group' the page scrolls to the bottom. All I can see is the circle icon and the text field:

[CanisLupus]

Whaaaat. x) Hmm, it doesn't happen here on Firefox 63, but I'll be sure to check what happens on more recent versions too. Thanks!

[CanisLupus]

I'm thinking that it's possible that you have settings/engines saved with your version of the branch, which may have a different format for the "color" variable in each group engine (rgba() instead of rrggbb). The code enables the popup but then crashes in the same method. Maybe.

Can you try the "reset engines" option?

[CanisLupus]

Something related that I can add to the README at some point:

I keep different Firefox profiles for normal usage and SSS development (and work on SSS in Firefox 63), so I tend to always get a clean storage each time. :)

While leaving tsc.cmd watching in the background for .ts changes and converting to .js, I also run this in a batch file:

web-ext run ^
    --source-dir "repository\src"^
    --firefox="C:\Program Files\Mozilla Firefox 63\firefox.exe"^
    --firefox-profile "%APPDATA%\Mozilla\Firefox\Profiles\b2i26c8l.Firefox 63 (Dev)"^
    --browser-console^
    --start-url www.mozilla.org^
    --ignore-files "**/*.ts"^

This runs the extension from "src", in Firefox 63, with profile "b2i26c8l.Firefox 63 (Dev)" (created beforehand) which is clean, opens the browser console automatically, opens a url for quick tests (Mozilla's website), and it auto reloads the extension each time there is a change (except in .ts files, since we must wait for tsc.cmd to convert then to .js after each change, and .js are the files that actually trigger the extension reload). web-ext also cleans all changes made when I exit Firefox, hence the clean storage.

[igorofc]

I'm thinking that it's possible that you have settings/engines saved with your version of the branch, which may have a different format for the "color" variable in each group engine (rgba() instead of rrggbb). The code enables the popup but then crashes in the same method. Maybe.

Can you try the "reset engines" option?

Tried that now but the same thing happens. I had also ran it via web-ext without success. Also happens when I test in Firefox 63. I'll test on a windows machine to see if it works.

[igorofc]

Something related that I can add to the README at some point:

I keep different Firefox profiles for normal usage and SSS development (and work on SSS in Firefox 63), so I tend to always get a clean storage each time. :)

While leaving tsc.cmd watching in the background for .ts changes and converting to .js, I also run this in a batch file:

web-ext run ^
  --source-dir "repository\src"^
  --firefox="C:\Program Files\Mozilla Firefox 63\firefox.exe"^
  --firefox-profile "%APPDATA%\Mozilla\Firefox\Profiles\b2i26c8l.Firefox 63 (Dev)"^
  --browser-console^
  --start-url www.mozilla.org^
  --ignore-files "**/*.ts"^

This runs the extension from "src", in Firefox 63, with profile "b2i26c8l.Firefox 63 (Dev)" (created beforehand) which is clean, opens the browser console automatically, opens a url for quick tests (Mozilla's website), and it auto reloads the extension each time there is a change (except in .ts files, since we must wait for tsc.cmd to convert then to .js after each change, and .js are the files that actually trigger the extension reload). web-ext also cleans all changes made when I exit Firefox, hence the clean storage.

That's good and can be really helpful. My machine is kinda slow so I tend to use about:debugging to avoid having a second instance of Firefox running which, along with VS Code, could render it even slower, although having to manually reload can quickly become a pain. But, sure, this definitely deserves to be in the README.

[CanisLupus]

Hi Daniel, nice to hear you're working on this. I know it's a lot of work for too little time and figuring out all that needs correcting must be a very strenuous job (so much for my amateurish coding skills). :)

You had setTimeout(() => groupTitleField.focus(), 100);. I haven't seen the timer being necessary, but I may be missing something. Do you know why this was needed?

Well, now that you mentioned it, I tested without the setTimeout and it focus just right. I most certainly had trouble with it in some other project and put it there without testing beforehand. No need for it then.

I'll pull the changes and check it out.

Thanks for the effort!

Totally forgot to reply to this, but I did read it. :) Sorry and thanks for checking the timeout. This feature also takes so long since it is one of the largest possible changes to SSS based on all the suggestions we got. :D

Tried that now but the same thing happens. I had also ran it via web-ext without success. Also happens when I test in Firefox 63. I'll test on a windows machine to see if it works.

Interesting!

That's good and can be really helpful. My machine is kinda slow so I tend to use about:debugging to avoid having a second instance of Firefox running which, along with VS Code, could render it even slower, although having to manually reload can quickly become a pain. But, sure, this definitely deserves to be in the README.

I see! If only our desktop apps weren't all built on browser engines nowadays, maybe we'd have nice things like... more available RAM. 🙄 about:debugging is a pain indeed, I can confirm.

[CanisLupus]

Hi Igor, I've finished the bug fixing and merged the changes to the develop branch. 🎉 :)

However, I still couldn't see the problem you were seeing, where the popup was at the bottom of the settings page. We may have to find out why that was happening.

[igorofc]

Hey, it's great that you're finished. I'm still having the same problem though. I haven't touched the code in a few days but I'll try to find out what's causing it here. When I loaded the extension on a windows machine something even weirder happened: first it didn't open the welcome tab, and also the settings page was blank. No errors were showing in the browser's console. I'll try on another windows laptop to see what happens.

[CanisLupus]

The scary part is having no errors. 🤔 Ok, thanks! But no rush. :)

[igorofc]

Oh, jeez, I completely forgot to do the transpiling. That's why I was having problems. My bad :facepalm:. I didn't test on the windows machine but it should work flawlessly now.

But, the tsc module is showing an error in line 216 of page-script.ts:

I did some digging and found out that replacing setTimeout with window.setTimeout - which returns a number - should do the trick.

Everything else is looking perfect. Well done!

[CanisLupus]

Hahah alright, then it's finished! ;) I've made a few more (unrelated) commits to "develop". When I have some time I'll test some more and release a new version.

Regarding the transpilation error you are seeing: thanks for the warning! I should probably not have left this part of the package.json like this:

Using ^3.0.1 allows more recent versions than that for TS, as long as it stays on a 3.x.x version. I'm guessing that yours installed a more recent version of TypeScript than mine; one in which the declaration of setTimeout was changed/fixed, so I don't see the error (mine returns "number") but yours is more up to date. It should be the exact same when "converted" to JavaScript, but I'll take a look and try to fix this.

EDIT: Or just commit the package-lock.json file, maybe. xD

[igorofc]

Using ^3.0.1 allows more recent versions than that for TS, as long as it stays on a 3.x.x version. I'm guessing that yours installed a more recent version of TypeScript than mine; one in which the declaration of setTimeout was changed/fixed, so I don't see the error (mine returns "number") but yours is more up to date. It should be the exact same when "converted" to JavaScript, but I'll take a look and try to fix this.

Ah, cool, I had no idea. It's all sorted then.

[CanisLupus]

OK, pushed a commit that adds the packages lock file. Also updated the dependencies to the most recent versions since it didn't seem to cause any problems. But that's the thing... it didn't cause any problems here. 🤔

So perhaps the problem you see is not related to the type definitions used in SSS (which are TypeScript defaults + types for WebExtensions), and instead something is defining setTimeout to return a value that is not a number (for example, Node.js type definitions). Now, how those would be used for SSS transpilation on your machine I don't know, because I only specify those 2. But perhaps you have more types defined globally? This is out of my area of expertise. 😁

I changed it to window.setTimeout anyway, to avoid future problems. :)

[igorofc]

Alright, I've updated the code and the error is gone. It's most likely that I got something wrong on my end, but I really don't know what it could be. Everything seems to be working just fine!

[CanisLupus]

You might have seen already, but 3.47.0 is now available with the new groups feature. I was just now replying to every issue this release affects (I got into the habit of writing replies beforehand to then just copy paste when the version goes live). Thanks again! :)

[CanisLupus]

And I'll be crossing fingers that the big changes didn't break anything. All features seemed to work (I tested them all at least once), but I don't like the unsure feeling I get about WebExtensions and JavaScript sometimes... i.e. Easy to make mistakes, hard to get feedback that you made one.

[igorofc]

Nice to see this finally going live. I've using a few groups myself and it's all working as it should. Let's hope people will enjoy the new feature. :tada:

[CanisLupus]

Great, happy to hear it! ;) 🎉