fix(call): Fix incorrectly selected media when actually joining a call

[nickvergessen]

☑️ Resolves

• Fix Blur state is inverted in media settings on page load #9501
• Ref is 27105c9
• But the change was not fully done, so the webrtc code still checked without videoDisabled_2790450198 but the media setting set nextcloud_per_dGFsaw==_videoDisabled_2790450198 now both paths are consistent.

❓ Questions

• We could also fix it the other way around and basically revert 27105c9 so it also writes without nextcloud_per_dGFsaw==_ again. The advantage would be that existing settings would be reused, which I would actually prefer.
• Or in case the value is null, we check the old browser storage name to fallback. But that might be very cumbersome.

🏁 Checklist

• ⛑️ Tests (unit and/or integration) are included or not required
• 📘 API documentation in docs/ has been updated or is not required
• 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required
• 🔖 Capability is added or not needed

[danxuliu]

localStorage is still used for media state in App.vue (to restore the state when switching to another room while in a call).

It is also used in a store test, but I have not checked for what (nor why the test do not fail), and to save the nick of guests.

Regarding whether to fully embrace BrowserStorage or go back to localStorage so existing settings are reused I have no preference; I guess that the move to BrowserStorage was done for some reason, so fine to move it, but reuse the existing settings is good, so fine to go back as well :-P

[nickvergessen]

and to save the nick of guests.

Yeah that has to stay, so text, collabora and talk use the same name.

Just trying to fix the regression between media settings and the actual call for now.

[nickvergessen]

For the background i guess the idea was only consistency
#9347 (comment)
So might also be okay to revert for the time being and then fully migrate with a transition in the future?

Cc @ShGKme

[ShGKme]

For the background I guess the idea was only consistency

At least consistency, as we have an abstraction for Nextcloud apps. The same as with Axios where we always use @nextcloud/axios instead of any other HTTP client or custom axios extension.

In the general case, it also helps to keep values in a specific app scope, less global between apps.

So might also be okay to revert for the time being and then fully migrate with a transition in the future?

Is the only problem - to keep the current local (in browser) settings for local media settings?

[nickvergessen]

Is the only problem - to keep the current local (in browser) settings for local media settings?

Well that would be why "reverting" would be better then "going forward with this PR".

At least consistency, as we have an abstraction for Nextcloud apps.

I get that, but it's also kind of annoying to break the setting.

But either thing needs to be done, so webrtc + media settings save/read from the same place. Currently whatever you do in the media settings is ignored in the call view and vice-versa

[nickvergessen]

But okay, then let's go forward with this PR :)

[ShGKme]

So might also be okay to revert for the time being and then fully migrate with a transition in the future?

If we want to add migration in the next major (28), then, because of a long cycle, it will be migrated in almost a year in 29...

Maybe add it before the 27 rc.1, so it will have a migration during 27 and then remove plain LS in 28?

Aka

1. Always use nextcloud's storage
2. On loading check if migration was done
3. If it wasn't - set nextcloud's storage to local storage, mark it as done

[nickvergessen]

So might also be okay to revert for the time being and then fully migrate with a transition in the future?

If we want to add migration to the next major (28), then, because of a long cycle, it will be migrated in almost a year in 29...

Yeah well, we could immediately remove the migration logic again when stable28 is branched off (if it hurts, but i guess it does not)
Basically on App.vue load we would check if there is any audioDisabled_* or videoDisabled_* entry in localStorage, if yes loop over all of them and add them to BrowserStorage and remove them from localStorage

[nickvergessen]

It is also used in a store test, but I have not checked for what (nor why the test do not fail),

@danxuliu So that works because BrowserStorage still uses localStorage and all the keys just have a prefix but the regex of the keys only checks for the end.

spreed/src/store/callViewStore.spec.js

Line 101 in 04c973e

expect(localStorage.getItem.mock.calls[0][0]).toEqual(expect.stringMatching(/callprefs-XXTOKENXX-isgrid$/))

If that would be changed to /^callprefs-XXTOKENXX-isgrid$/ it would fail with:

    Expected: StringMatching /^callprefs-XXTOKENXX-isgrid$/
    Received: "nextcloud_per_dGFsaw==_callprefs-XXTOKENXX-isgrid"

But the missing leading ^ means it just checks for the end, so all good.

[ShGKme]

Except for one issue, all works fine and looks good.

Also checked that no other storage item is missed.