-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 Hide app icon
option
#2462
base: master
Are you sure you want to change the base?
Add Hide app icon
option
#2462
Conversation
Hide icon from launcher
optionHide app icon
option
85b2d24
to
39c349b
Compare
The icon should be enabled by default. These two should seems to do the opposite: |
Also in my opinion the variable name seems a bit strange: Personally I would choose |
The option in microG settings is disabled so the icon should be enabled by default, but you disable the icon in reality here:
Edit: Now that I read better, here you don't disable the icon but the MainSettingsActivity so you actually break the microG settings. This shouldn't be touched => org.microg.gms.ui.MainSettingsActivity |
play-services-base/core/src/main/kotlin/org/microg/gms/settings/SettingsProvider.kt
Outdated
Show resolved
Hide resolved
This is just personal opinion but in this case: instead with this: So actually it is the same in practice but I think the first is just more confusing but it is still personal preference. @mar-v-in What do you think? |
75e6627
to
6f4e97f
Compare
@ale5000-git, I believe I have addressed all the requested changes in this pull request. Please let me know if there are any additional modifications or suggestions. |
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.
See my comments on the code. Also, please add a way to hide this setting on the Huawei build flavor, as devices with Huawei OS that support microG, the microG settings is automatically hidden from launcher.
@@ -75,6 +76,7 @@ | |||
import static android.view.View.VISIBLE; | |||
import static android.view.inputmethod.InputMethodManager.SHOW_IMPLICIT; | |||
import static org.microg.gms.auth.AuthPrefs.isAuthVisible; | |||
import static org.microg.gms.checkin.CheckinPreferences.hideAppIcon; |
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.
You add new imports without any need. Please remove them again
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 PR made changes to play-services-core/src/main/kotlin/org/microg/gms/checkin/CheckinPreferences.kt and play-services-base/core/src/main/kotlin/org/microg/gms/ui/Utils.kt?
android:icon="@drawable/ic_hide_app_icon" | ||
android:key="hideAppIcon" | ||
android:summary="@string/pref_hide_app_icon_summary" | ||
android:title="@string/pref_hide_app_icon_title" /> |
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.
Preferences should not be directly modified from the :ui
process as shared preferences are not safe for multi-process use. That's why we generally use the SettingsProvider
to store settings.
However, in this case, the settings provider is not needed, as the ground truth of the component enabled state is in the PackageManager. With your code, if someone enables or disables the component without using this setting (e.g. by directly invoking pm disable
via adb
), the setting would no longer match the actual live state.
I suggest you make the setting non-persistent (by settings android:persistent="false"
) and configure it from code by taking the value from getComponentEnabledSetting
when realizing the setting.
@@ -65,9 +65,15 @@ | |||
android:title="@string/service_name_location"/> | |||
</PreferenceCategory> | |||
<PreferenceCategory android:layout="@layout/preference_category_no_label" android:key="prefcat_footer"> | |||
<SwitchPreferenceCompat |
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 don't think this setting is so important that it deserves a prime location at the top level settings dialog. I suggest to create a new section for these kind of things, although I don't know how to name it or what else would belong there.
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.
@mar-v-in
Some ideas:
- Other settings
- Minor settings
- Trivial settings
- UI settings
@@ -34,6 +34,7 @@ object SettingsContract { | |||
const val SECURITY_TOKEN = "securityToken" | |||
const val VERSION_INFO = "versionInfo" | |||
const val DEVICE_DATA_VERSION_INFO = "deviceDataVersionInfo" | |||
const val HIDE_APP_ICON = "hideAppIcon" |
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 does not belong into the Checkin settings contract. In fact it's not needed at all in SettingsContract
or SettingsProvider
, see my other comment
@mar-v-in |
This reverts commit 3094aff.
Adds an option to hide icon from launcher.
Resolves #2263, resolves #2266 as well as resolves #2361.
Credits: @rufusin