-
-
Notifications
You must be signed in to change notification settings - Fork 85
Add deny browser to open/share functionality #523
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: master
Are you sure you want to change the base?
Add deny browser to open/share functionality #523
Conversation
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.
Hi, thanks for your contribution!
As you have probably noticed, I'm currently a bit absent, so apologize in advance if it takes me several days to reply. Feel free to ping me though (but don't abuse it, please).
I wrote some comments about the new functionality, try to follow them, but feel free to discuss if you think otherwise.
| /* ------------------- Buttons ------------------- */ | ||
|
|
||
| /** Shares the url as text */ | ||
| public void shareUrl() { |
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.
I understand this change, but for now I prefer to leave the share as just 'call android', without extra logic. It may change in the future though, but it will require other functionality present in the open that is still not here (like reordering)
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:gravity="center_vertical" | ||
| android:text="Deny Browser" /> |
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.
move this text to the translations file (place it under mOpen_iconSize)
| */ | ||
|
|
||
| public enum DenylistOption implements Enums.IdEnum, Enums.StringEnum { |
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.
Even though this is a good first implementation, I wonder if it's too restrictive.
You can only choose from a predefined list, and you can't choose more than one.
I prefer to store a multilinestring, where each line is one package (empty or invalid lines ignored). This will either require implement a chooser dialog dynamically loaded with the apps...or just leave the textarea and let the user write the packages (not user friendly, I know, but more powerful).
Or, even better, with something similar to the 'templates' in the webhook module. Yes, I think this will be easy to implement, and could be easily replaced with a more powerful dialog if needed.
|
This PR builds correctly, here is the generated apk.
You must be logged in for the link to work. This is an automatic comment created by a Github Action based on a previous Github Action |
I saw that someone had requested for there to be less browsers based on user preference so I was able to add something that removed one browser from chrome, edge, opera, brave and Firefox that user did not like. In future, could update to include more apps and maybe even increase the amount of apps available to deny at a single time to be an actual list.