Skip to content
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 Miscellaneous setting and Hide app icon option #2462

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ILoveOpenSourceApplications
Copy link

@ILoveOpenSourceApplications ILoveOpenSourceApplications commented Jul 30, 2024

Adds an option to hide icon from launcher.

Resolves #2263, resolves #2266 as well as resolves #2361.

Credits: @rufusin

@ILoveOpenSourceApplications ILoveOpenSourceApplications changed the title Add Hide icon from launcher option Add Hide app icon option Jul 30, 2024
@ale5000-git
Copy link
Member

The icon should be enabled by default.

These two should seems to do the opposite:
CheckIn.HIDE_APP_ICON -> getSettingsBoolean(key, true)
android:enabled="false"

@ale5000-git
Copy link
Member

ale5000-git commented Jul 30, 2024

Also in my opinion the variable name seems a bit strange:
private fun updateHideAppIcon(enabled: Boolean)

Personally I would choose
private fun updateHideAppIcon(hide: Boolean)
or
private fun updateShowAppIcon(enabled: Boolean)

@ale5000-git
Copy link
Member

ale5000-git commented Jul 31, 2024

The icon should be enabled by default.

android:defaultValue="false"/>

The option in microG settings is disabled so the icon should be enabled by default, but you disable the icon in reality here:
https://github.com/microg/GmsCore/pull/2462/files#diff-d629a5125b2f479f4916b4b963c3ed5bc5b8c8e79951dfb805bc1fa4a7384624R597-R605

        <activity
            android:name="org.microg.gms.ui.MainSettingsActivity"
            android:enabled="false"

Edit: Now that I read better, here you don't disable the icon but the MainSettingsActivity so you actually break the microG settings.
This one mustn't be disabled in any case.

This shouldn't be touched => org.microg.gms.ui.MainSettingsActivity
This can be disabled (but not by default) => org.microg.gms.ui.SettingsActivity

@ale5000-git
Copy link
Member

ale5000-git commented Jul 31, 2024

Also in my opinion the variable name seems a bit strange: private fun updateHideAppIcon(enabled: Boolean)

May I ask what the variable does here so that I could understand which one from the below two would make sense?

This is just personal opinion but in this case:
updateHideAppIcon(enabled=true)
when enabled = true, the icon is hidden so not enabled.

instead with this:
updateHideAppIcon(hide=true)
when hide = true, the icon is hidden so not enabled.

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?

@ILoveOpenSourceApplications
Copy link
Author

@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.
@mar-v-in, your input would also be greatly appreciated.
Thank you for your time and consideration.

Copy link
Member

@mar-v-in mar-v-in left a 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.

@ale5000-git
Copy link
Member

@mar-v-in
I think it would also be nice to be able to optionally set the default status in /system/etc/microg.xml, so ROM authors and flashable ZIPs can set it without the need to mess with it in other ways.

@ILoveOpenSourceApplications
Copy link
Author

I have made changes according to the requests, not sure if it is what was intended. Do let me know if further changes are needed.

@ILoveOpenSourceApplications
Copy link
Author

@mar-v-in I think it would also be nice to be able to optionally set the default status in /system/etc/microg.xml, so ROM authors and flashable ZIPs can set it without the need to mess with it in other ways.

Is this within the scope of this PR or just a suggestion for the future of microg?

@ale5000-git
Copy link
Member

Is this within the scope of this PR or just a suggestion for the future of microg?

It can also be done later in a different PR.

@ale5000-git
Copy link
Member

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.

@ILoveOpenSourceApplications
You misunderstood this, this should happen on Huawei build flavor, it is a flavor of microG; it shouldn't check the device manufacturer.

@ale5000-git
Copy link
Member

Also, although I haven't tried, the changes to AndroidManifest.xml seems totally unnecessary.

@ILoveOpenSourceApplications
Copy link
Author

@ILoveOpenSourceApplications You misunderstood this, this should happen on Huawei build flavor, it is a flavor of microG; it shouldn't check the device manufacturer.

Can you point me as to where I need to make the changes?

@ILoveOpenSourceApplications ILoveOpenSourceApplications changed the title Add Hide app icon option Add Miscellaneous setting and Hide app icon option Oct 31, 2024
@ILoveOpenSourceApplications ILoveOpenSourceApplications marked this pull request as draft November 1, 2024 15:10
@ILoveOpenSourceApplications ILoveOpenSourceApplications changed the title Add Miscellaneous setting and Hide app icon option Add Miscellaneous setting and Hide app icon option Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants