-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(mobile): add read only mode #19368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b74843b
to
7601daa
Compare
Thank you for the PR, however, we are likely not going to accept this as it is quite a niche use case |
ha fair enough but can we maybe just see if people want it ? I get it that it's on the app bar settings page where it is very center and distracting but is there somewhere else that I can put this with ?
This would really make a nice differentiating feature from all the current apps (not that they can implement it for themselves) but this is coming in from a few folks that I've had interactions with and there is immich which is widely being adopted & my daily driver now.
Is there maybe a way just to ask more people either on discord or somewhere else to see if we can have it ?
|
Let's call this a read-only mode, and brainstorm on where to put a quick toggle that is still having a good UI |
@Sud-Puth This is great! I had a few ideas on ways we can compromise on feature visibility and quick access to the setting for parents. I am thinking that it would be best to put a toggle in the advanced settings pane that controls the setting. Beneath the title, we can put a little description that explains an easier method to quickly enable/disable it. That "easier" method would then be something akin to double-tapping the user icon would toggle the read-only mode. We would put a small banner in the user dialog saying "Read-only mode enabled" with a description "Double-tap the user icon to exit". This allows us to keep the user dialog simple while still having a quick way to enable and disable it when a parent gives their phone to their kid. |
@bwees -- love that idea ! will push those changes up soon |
Thank you! I will work with you on this to get it polished then we can get Alex for the final review. |
@bwees - give it a whirl now |
e1c4376
to
0ae277a
Compare
So I don't like the Red color for background nor text -
Check the latest commit - 0ae277a |
@bwees - Check it out - made those updates |
Thank you! I think we are good to hide the select icon for the rows when in readonly. You should be able to add a conditional in group_divider_tile.dart:
I also posted a few code review changes to match our code style a bit better. Once these are complete I think its good for Alex to review! |
Awesome thanks! I will take a look at this first thing tomorrow and let you know if there are any last changes before final review |
Looks like the translation file needs alphabetized, can you run |
@bwees - all done - will monitor for sometime for the checks & will come back later. Thanks for all the things ! |
@bwees - one last change for the dart analysis -
not sure why the local dart analysis wouldn't fail fwiw |
Awesome thanks, we just added DCM rules to out CI actions yesterday so your local machine might not be setup to run them. This should fix it. |
Just testing the PR, and I have a few comments
|
@alextran1502 - just a reminder for a review whenever - did not want this to fall out of your view |
@Sud-Puth Thanks for the reminder! can you allow maintainers to push to this PR? I might modify a few things regarding the styling before merging it |
@alextran1502 - done |
@@ -203,26 +205,38 @@ class TopControlAppBar extends HookConsumerWidget { | |||
actionsIconTheme: const IconThemeData(size: iconSize), | |||
shape: const Border(), | |||
actions: [ | |||
if (asset.isRemote && isOwner) buildFavoriteButton(a), | |||
if (asset.isRemote && isOwner && !isReadonlyModeEnabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hiding the individual component action, maybe just hiding the entire top/bottom control bar would be cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense but I was considering on a what-if scenario if we wanted to implement and show something on read-only mode but I can def. remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh also the back button is gone if we remove the top control app bar, the back button still works though.
@alextran1502 - Updated/Addressed |
@Sud-Puth this is on our list to review. Thanks for working through these changes with us! |
Not forgetting you, been busy with other works, will take a look shortly |
@Sud-Puth thanks for your patience on this feature, we have been hard at work on a lot of performance improvements in the mobile app. As part of this, we are preparing to release an improved timeline view and asset viewer (both places which this feature touches). I am going to migrate these changes to the new interface components and this feature will release alongside them as a new feature. All of the backend you implemented is very good and will be used, I am just going to migrate the control visibility conditionals to the new interface. All changes will be attached to this PR so it stays contained and credit for work is retained. Thanks again for your hard work and patience on this feature, I'm excited to get this merged! |
@bwees - Appreciate the constant updates and thank you for getting all things updated . Hopefully this is going to be used as much as I am using the debug app. 🤞 Def. find it much useful on a daily usage |
Glad to hear it, we are finishing up this massive mobile rewrite then hopefully we can get it into the review pipeline. I will do my best to keep you/this thread updated with the status. |
Yup - I keep seeing the PRs - let me know if I can help with my time in any way |
if you want, you can join our discord and check out the #contributing channel. Discussions happen there where people pick up bugfixes or features. |
0f33486
to
c357146
Compare
This commit introduces a "Kid (Readonly) Mode" feature. - Adds a `KidModeProvider` to manage the state of Kid Mode. - Implements a `KidModeCheckbox` widget in the app bar dialog to toggle Kid Mode. - When Kid Mode is enabled, - Disables selecting the multigrid & the bottom bar - Removes the top bar from view Signed-off-by: Sudheer Puthana <[email protected]> Reverts the changes to devtools_options.yaml file Signed-off-by: Sudheer Puthana <[email protected]> refactor: replace Kid Mode with Readonly Mode This commit replaces the "Kid Mode" feature with a more generic "Readonly Mode". - Renamed `KidModeProvider` to `ReadonlyModeProvider`. - Readonly Mode state is now persisted in app settings. - Added a new app setting `allowUserAvatarOverride` to toggle read-only mode. - Updated translations. - Added a message in the app bar dialog indicating when read-only mode is active. Signed-off-by: Sudheer Puthana <[email protected]> Address comments - - Removes the `allowUserAvatarOverride` setting. - Hides the bottom gallery bar when read-only mode is enabled. - Adds an icon on the main app bar when read-only mode is enabled with a snackbar. Signed-off-by: Sudheer Puthana <[email protected]> Update to snackbar - When toggling readonly mode from either the settings or the app bar, a snackbar notification will now appear. - The readonly mode message in the profile drawer has been restyled. - The upload button in the app bar is now hidden when readonly mode is enabled. Signed-off-by: Sudheer Puthana <[email protected]> Removes clearing of snackbar Signed-off-by: Sudheer Puthana <[email protected]> Address Comments - Consolidated snackbar messages for enabling/disabling readonly mode. - Ensured the "Select All" icon in asset group titles is hidden in readonly mode. Signed-off-by: Sudheer Puthana <[email protected]> Adds in the missing translation keys for readonly_mode Signed-off-by: Sudheer Puthana <[email protected]> Fix translation Signed-off-by: Sudheer Puthana <[email protected]> Fix check failure for BorderRadius Signed-off-by: Sudheer Puthana <[email protected]> Changes: - Adjusted AppBar background color in readonly mode. - Removes cross-out pencil icon button in favor of above. - Hides the "Edit" icon next to date/time, disable description and onTap for people and location when readonly mode is enabled. Signed-off-by: Sudheer Puthana <[email protected]> Address comments from Alex - Moved readonly mode check to `GalleryAppBar` to hide the entire `TopControlAppBar` when readonly mode is enabled. - Changed `toggleReadonlyMode` in `ImmichAppBar` to directly toggle the state. Signed-off-by: Sudheer Puthana <[email protected]> migrate readonly mode to new beta timeline remove readonly mode from legacy UI only show readonly functionality when on beta timeline simplify selection icon update generated provider chore: more formatting
Description
Introduces a "Read-only Mode" feature.
allowUserAvatarOverride
to toggle read-only mode via the user avatar icon.In total just enables the users to give the Immich app on their mobile to their little loved ones without them deleting nor pressing something that they shouldn't have.
How Has This Been Tested?
Checklist:
src/services/
uses repositories implementations for database calls, filesystem operations, etc.src/repositories/
is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/
)