(Tizen) Add logout to exit menu #5271

RoboMagus · 2024-03-09T11:17:10Z

Changes
Adds Sign Out option to Tizen TV exit menu to mitigate the hassle of switching users that would otherwise require much more interaction.

Users would either have to:

Fully close the app and start it back up again
- This has wait time, especially on older TVs (like mine)
- Only really works nicely when users are configured to Not be remembered and have no password configured
Logout through the menu
- This requires at least 16 remote interactions. Which is just too much of a hassle.
  - From the top of the Home Screen; Up (1), Right(4), Ok(1), Down(9), Ok(1)

dmitrylyzo

Tbf, I think it would be better to fix Remember Me behaviour:

Use sessionStore (or session cookies) to store the credentials.
Make Remember Me unchecked by default. This requires 1. Otherwise, opening links in the new tab doesn't sign in.
Uncheck Remember Me when password-less graphical login is used.

This way, you stay logged in until you close the app/browser.

My efforts:
#2481
jellyfin-archive/jellyfin-apiclient-javascript#154

ApiClient is deprecated and we are moving to React, so I am not sure how to do all of the above.

dmitrylyzo · 2024-03-09T13:45:13Z

src/components/apphost.js

@@ -308,12 +308,17 @@ function askForExit() {
        exitPromise = actionsheet.show({
            title: globalize.translate('MessageConfirmAppExit'),
            items: [
+                { id: 'logout', name: globalize.translate('ButtonSignOut') },


Shouldn't logout menu entry be added only if the user is logged in?

Also, I don't think logout should be the default action.

Shouldn't logout menu entry be added only if the user is logged in?

Good point, I should add a check for that.

Also, I don't think logout should be the default action.

That could be a point for discussion indeed. IMO not many people would actually go through this menu to close the app on their TV, if the app even is fully closed at all. More users would probably be switching between accounts.
Seeing as putting this option between 'Yes' and 'No' does not feel right aesthetically, and way at the bottom messes with the UX of having cancel / ignore as the always last option this seemed to be the best place to keep it for now.

IMO not many people would actually go through this menu to close the app on their TV, if the app even is fully closed at all.

This menu is what Samsung suggests for exiting the app on Back action. And I think it does close it compared to opening another app - it doesn't restore the state when re-launching.

Seeing as putting this option between 'Yes' and 'No' does not feel right aesthetically, and way at the bottom messes with the UX of having cancel / ignore as the always last option this seemed to be the best place to keep it for now.

In fact, No should be the default.
It can be marked as selected:

jellyfin-web/src/components/actionSheet/actionSheet.ts

Line 20 in 218db7a

selected?: boolean;

This menu is what Samsung suggests for exiting the app on Back action. And I think it does close it compared to opening another app - it doesn't restore the state when re-launching.

That is the way this menu currently functions, yes. What I'm suggesting is that most people are unlikely to close the app using this menu. Either the app is suspended into the background when a new app is opened, or if the app actually needs to be closed completely users would be likely to force-close it rather than going through a menu.

It can be marked as selected:

I've been testing with that, but it seems this option only puts a checkmark in front of the item. Not actually highlighting it as the active selection.

would be likely to force-close it

Force-close is the last option to exit, not the correct way to close the app.

Not actually highlighting it as the active selection.

In Chrome (device mode to emulate Tizen + a hack to call the menu) it is highlighted, but the checkmark is not desirable. We need to add an option that allows us to set the current menu item.

Force-close is the last option to exit, not the correct way to close the app.

Based on my experience with end users they don't care about the "correct way", but rather most convenient / easiest / or fastest way. Though that's a bit beside the point, it does illustrate why I believe the exit menu would be a good place to add a logout option. It's much more convenient than navigating through the menu to log out, and it's a better option to force closing the app to switch accounts.

I'm fine with discussing placement / sorting of the options in the exit menu and which should be highlighted by default. Though if changes need to be made to the way these menus are currently implemented that would best be handled in another issue.

RoboMagus · 2024-03-09T15:21:07Z

Tbf, I think it would be better to fix Remember Me behaviour

I'd love to see improvements to the Login + Remember Me behavior, but that would require too many changes and must be done properly. A task I'm currently not able to take on.
Ideally I'd like to have decent password protection on my accounts whilst still being able to easily switch users on the TV without having to sign in and out. However as it stands now I believe it's not possible for users to have multiple users logged in and 'remembered' at the same time on the same client (Though I may be wrong about this?). Making it impossible to both have decent password protection of accounts and easy user switching without having to re-authenticate.
Though that is all a bit too far off topic...

The main thing this PR adds though is easier user switching. Which is still a separate issue from the remember me state of each user.
In my case, the Remember Me option is unchecked for all active users on the TV so when the app starts cold, we get to select which user to continue with.
However, this only is the case for a cold start of the app. Not when switching apps, and also not when the TV has been powered off. In both those cases the user is still retained as the app is not closed fully.

github-actions

ESLint doesn't pass. Please fix all ESLint issues.

github-actions · 2024-03-09T15:49:59Z

src/components/apphost.js

-            exitPromise = null;
+    import('../utils/dashboard').then(() => {
+        import('../components/actionSheet/actionSheet').then((actionsheet) => {
+            const userId = Dashboard.getCurrentUserId();


'Dashboard' is not defined. no-undef

github-actions · 2024-03-09T15:49:59Z

src/components/apphost.js

+                if (value === 'yes') {
+                    doExit();
+                } else if (value === 'logout') {
+                    Dashboard.logout();


'Dashboard' is not defined. no-undef

github-actions

ESLint doesn't pass. Please fix all ESLint issues.

src/components/apphost.js

sonarqubecloud · 2024-03-11T12:49:56Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions

ESLint doesn't pass. Please fix all ESLint issues.

dmitrylyzo · 2024-03-12T13:21:31Z

src/components/apphost.js

-            }
-        }).finally(function () {
-            exitPromise = null;
+    import('../utils/dashboard').then(() => {


At least use the result.

Suggested change

import('../utils/dashboard').then(() => {

import('../utils/dashboard').then((Dashboard) => {

Should we use camelCase (dashboard) since it is a variable? 🤔

utils/dashboard exports Dashboards so it is available for use without grabbing the result.

Tested on TV and working as intended.

Yes, it works, but there are ESLint errors.

I asked the team and got the answer: We should use the import in our code. The global is for use in places outside of web (plugins, wrapper apps, etc).

Add logout to exit menu

0a42aa1

RoboMagus requested a review from a team as a code owner March 9, 2024 11:17

dmitrylyzo added feature New feature or request p: tizen This PR or issue mainly concerns Tizen clients labels Mar 9, 2024

dmitrylyzo reviewed Mar 9, 2024

View reviewed changes

github-actions bot requested changes Mar 9, 2024

View reviewed changes

Only show logout option if user is logged in

4388e5e

RoboMagus force-pushed the tizen_exit_mod branch from b895aaa to 4388e5e Compare March 9, 2024 16:57

github-actions bot requested changes Mar 9, 2024

View reviewed changes

src/components/apphost.js Outdated Show resolved Hide resolved

RoboMagus mentioned this pull request Mar 9, 2024

Change default AutoLogin #5272

Open

Fix dashboard import

5f137b1

github-actions bot requested changes Mar 11, 2024

View reviewed changes

dmitrylyzo reviewed Mar 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Tizen) Add logout to exit menu #5271

(Tizen) Add logout to exit menu #5271

RoboMagus commented Mar 9, 2024

dmitrylyzo left a comment

dmitrylyzo Mar 9, 2024

RoboMagus Mar 9, 2024

dmitrylyzo Mar 9, 2024

RoboMagus Mar 9, 2024 •

edited

Loading

dmitrylyzo Mar 12, 2024

RoboMagus Mar 13, 2024

RoboMagus commented Mar 9, 2024

github-actions bot left a comment

github-actions bot Mar 9, 2024

github-actions bot Mar 9, 2024

github-actions bot left a comment

sonarqubecloud bot commented Mar 11, 2024

github-actions bot left a comment

dmitrylyzo Mar 12, 2024

RoboMagus Mar 13, 2024

dmitrylyzo Mar 14, 2024

	import('../utils/dashboard').then(() => {
	import('../utils/dashboard').then((Dashboard) => {

(Tizen) Add logout to exit menu #5271

Are you sure you want to change the base?

(Tizen) Add logout to exit menu #5271

Conversation

RoboMagus commented Mar 9, 2024

dmitrylyzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoboMagus Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoboMagus commented Mar 9, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Mar 9, 2024

Choose a reason for hiding this comment

github-actions bot Mar 9, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 11, 2024

Quality Gate passed

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoboMagus Mar 9, 2024 •

edited

Loading