Temporary-containers: Keyboard shortcut to enable/disable isolation

[Arthaey]

If we had a keyboard shortcut to quickly toggle Isolation on and off, it wouldn't be quite as annoying a "dance" to add a new domain to a Multi-Account Container.

Perhaps this could be a (relatively) easy stopgap measure for #170, #282, #292, etc?

[stoically]

Not sure if I want to add yet another keyboard shortcut, since there's already 6 of them.

As a side-note in case you didn't know: you can enable/disable isolation in the popup top-nav.

[Arthaey]

Understood about not wanting to add another shortcut, even an "unset by default" one.

I guess I'll just have to make a fork for myself. :)

I do know about the popup, but thanks for making sure! It does make it quicker than having to open the full preferences page.

[stoically]

Well, I guess having a 7th, disabled by default, shortcut, especially one for disabling isolation, might not be a bad idea after all (and Alt+I is free - on Ubuntu at least). The only thing I'd be concerned about is that it might lead to accidentally disabling isolation compared to the popup which needs explicit interaction.

And yeah, I remember now, you even complimented the choice of badge for disabled isolation :D

[mdhooge]

"help wanted": Just have a look at the code… I think I can do it.
But just to be sure: is it enough to toggle "app.preferences.isolation.active"?

[stoically]

@mdhooge Yep, should be enough - and thanks!

[mdhooge]

So, I did the easy part: modify the UI:
https://github.com/mdhooge/temporary-containers/commit/9c9a4ab2128a03f423f9bffb6fbb767b0d5187c1

But the icon badge isn't updated :-(
I tried to call Preferences::handleChanges or directly copy/paste the calls to the [un]setPopup methods, but with no correct results.

I haven't coded with JS for a couple of years and I started learning how to develop an add-on with the MDN… If you have a hint to guide me, that'd be great!

M.

[stoically]

Looks good so far!

this.pref is a read-only proxy, to actually modify the preferences you'd have to change this.storage.local.preferences.

Btw, I'd prefer if the keyboardShortcuts key order and onCommand switch order would match the preferences UI.

[stoically]

And yeah, to get the icon badge updated, you'd also have to do what preferences.js handleChanges is doing in that case.

[mdhooge]

OK, now it works! https://github.com/mdhooge/temporary-containers/tree/Issue296

But before I pull a request, can you explain further your comment about shortcuts order? AFAIK it is the case. I chose to put it first because I thought it was logical. But I'll be happy to do as you wish.

I am not totally happy with the explanation text. And I just realised the text "Enable in Advanced Preferences > Keyboard shortcuts" is missing and must be added…

[stoically]

Nice. What I meant is that I'd prefer if the new shortcut would be last in the list, in all affected files, since it was added last. The hint in the manifest is indeed missing, and you might want to consider something along the lines of

      this.storage.local.preferences.isolation.active = !this.pref.isolation.active;
      if (this.pref.isolation.active) {
        this.background.browseraction.addIsolationInactiveBadge();
      } else {
        this.background.browseraction.removeIsolationInactiveBadge();
      }

for the commands.js. var isn't really used anymore in javascript, it's now let or const.

[mdhooge]

Another step toward achievement!
But the Enable/Disable icon in the popup is not updated :-(
And it's kind of magic to me, because its toggleIsolation method doesn't need refreshing the badge… I still didn't grasp how this is happening :cry:

PS1: For var, I remembered there was something special but…
PS2: I wondered if you wrote it on purpose… but I coded the if statement for the badge the other way round and it seems to be OK :smile:

[stoically]

Uh, yeah, if/else was indeed the wrong order, hah. toggleIsolation in the popup (UI) sends the savePreference message to the background.js which in turn calls the preferences.js handleChanges which then changes the badge. Let me know if there is something else.

Oh and, if you meant that the icon in the popup doesn't change when you press the shortcut while the popup is open, then that's something that would need more involvement (sending a message from the background to the popup). I'd say that's not really needed since the icon badge updates, and if you use the shortcut you probably don't open the popup anyway. Also if you open it after pressing the shortcut, the correct icon should show.

[mdhooge]

Also if you open it after pressing the shortcut, the correct icon should show.

:arrow_right: Unfortunately, no :cry:

I grep-ed and dug in the source code… but with my current level in add-on development, I can't figure out what's not happening. I read that vue.js already upgrade the HTML when there is a change. But I guess the change doesn't reach it.

[stoically]

Oooh, yeah, since you changed the preferences it needs a this.storage.persist(); after the the change so it's actually stored. The UI directly gets the storage and doesn't ask the backend runtime for that.

[mdhooge]

So I added the persist call to my issue branch (3 commits in total) and now this is nearly perfect (i.e. still not updating the pop-up ONLY IF it is already open).
Note that Travis failed but it is due to the brand new LTS node.js (v12.13 — see #313)

[mdhooge]

A quick follow-up, now that I have a better understanding of the whole add-on! :smiley:

• I didn't await on the storage.persist… but I believe this isn't important — so far.
• I found that options.vue listens to preferencesUpdated and I can trigger a _refresh_ of the options page by sending that message when toggling. Do you think it could be possible to listen to that message (or maybe a more _narrow_ one) from popup.vue?

[stoically]

I didn't await on the storage.persist… but I believe this isn't important — so far.

Yeah, shouldn't matter, since we have no real way to inform the user that something went wrong while persisting in case of just pressing the shortcut, and the isolation flag gets flipped without persisting, so the desired behavior should occur anyway.

Do you think it could be possible to listen to that message (or maybe a more narrow one) from popup.vue?

Good idea, and could work yeah. In theory it should be enough to move the listener from the options.vue into the root.js.

[stoically]

Added in v1.4beta1, thanks @mdhooge.

[stoically]

Jut wanted to say, I'm an avid user of that shortcut now :sweat_smile: