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
Add support for managing shortcuts to navigate to specific pages #1460
Conversation
Now that #1446 is merged I can confirm that |
I did a quick test.
|
Maybe leave the edit field for now to not confuse the user by too many options. |
} | ||
|
||
pinPreference?.setOnPreferenceClickListener { | ||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O && shortcutManager.isRequestPinShortcutSupported) { |
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.
This condition is used really often. Maybe it is possible to refactor it as method.
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.
So the first case this condition is used is for the 5 hard coded dynamic shortcuts to show that they can be pinned (if supported) The next time this condition is used is to hide the pinned category if the users device does not support it. I didnt see a good way to refactor this since the underlying code is different
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.
actually there is more than 2 instances but has a different purpose, will need to see if i can refactor some of them
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.
Something like this:
Instead of
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O && shortcutManager.isRequestPinShortcutSupported)
something like this one:
if (pinShortcutsSupported())
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.
Now that we are removing the ability to pin the dynamic shortcuts there is only one instance of this check :) So doesnt make sense but a lot less code now
That sounds to be expected if your updating from one PR build to another, webview reloading might be due to no background permission? I normally dont grant it myself when im beta testing a PR :)
So the reason there are 2 optoins is because dynamic shortcuts is in android 7.0 and pinned shortcuts is in android 8.0 so not all users will see everything depends on the device. Also dynamic shortcuts only show up under the app long press action and for that reason I opted to keep Add Shortcut and Pin Shortcut separate since its technically separate functionality eventhough shortcuts are identical. Renaming the label might be good to make it more clear but for users who only support dynamic they will only see dynamic controls and nothing related to pins.
One thing we could do is to mimic what was done with pinned shortcuts as we show a listview of all pinned items, and put the amount of dynamic shortcuts fully in user control. I opted to do 5 fields to make it easy on us and the user.
This is already done but if you are using one of the 5 categories above it you won't see the listview, it has to be a new unique ID for it to be editable. I did this because all users on android 7+ will have access to those 5 shortcuts and users with android 8 have the option to add more. Once a new shortcut is added using the below category the listview will appear and populate. For the 5 shortcuts above we have hardcoded ID's to make it predictable in the app.
I like linking to companion docs, might add a link under the instructions now that I think about it :) |
The debug app has the same permissions as the productive app. Even if i open the app instantly after i "closed" it by going to the homescreen, the webview will reload. Maybe this is normal in the debug app? Dunno.
Yes i would keep it separate. But for every dynamic shortcut you can also create a pinned shortcut
That would be also a possible solution for other areas in the settings. |
Ok will keep it separate to lessen confusion. |
4832788
to
55cee63
Compare
Summary
Fixes: #918 let's close the issue for now and let users add additional enhancement requests
Got what I think is a good MVP for supporting shortcuts by first focusing on navigating to a specific page in home assistant.
Here is what is entailed:
In the future:
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#482
Any other notes
https://developer.android.com/guide/topics/ui/shortcuts
Replaced a few hard coded strings with translated strings in the Settings fragment