OpenLauncherTeam / openlauncher

Customizable and Open Source Launcher for Android
Apache License 2.0
1.43k stars 413 forks source link

[Fixes #523] Remove uninstalled or hidden apps from groups. !Requires #580 #561

Closed cmtjk closed 4 years ago

cmtjk commented 4 years ago

Fixes: https://github.com/OpenLauncherTeam/openlauncher/issues/523 (@just-Nob may tell if this also fixes the F-Droid updates)

Rational

According to https://github.com/OpenLauncherTeam/openlauncher/issues/523 groups are currently unable to handle missing items (ie. uninstalled or hidden apps).

This PR fixes this issue by removing missing items.

update_group

Notes

  1. Group is not updated automatically after hiding/uninstalling an app.
    • This may be implemented in the future but requires more effort. Currently, we have to deal with the broken group icons till the user clicks on it.
  2. When app is unhidden again, it does not appear in it previous groups.
    • People my expect previously hidden apps to appear in their groups again but the current implementation doesn't allow this, because it introduces several new issues like stacked icons, seemingly empty groups, or oversized group popups.
just-Nob commented 4 years ago

Hi @r3r57 ,

I'll check it out as soon as the version including that fix will be available in the F-Droid Store. It also may take a little time then, because I have to guess which of my installed F-Droid apps will be updated next (or even regularly) to put it in a group folder (but I still have an idea).

dkanada commented 4 years ago

The update issues when installing or uninstalling apps only happens on newer phones, because the AppUpdateReceiver doesn't work for the work profile apps.

cmtjk commented 4 years ago

I've encountered an issue with this PR where valid apps where removed. This occured twice since 15th January, so it's quite rare. I think it's due to a race condition and I'll try to find the cause and fix it.

dkanada commented 4 years ago

Did you still want to find the race condition? If it's rare I see no problem merging this, sounds like an improvement.

cmtjk commented 4 years ago

@dkanada I didn't had enough time to debug it but I strongly recommend not to merge this yet since this happens regularly. Unfortunately, I was not able to reproduce the issue on purpose so it's really hard to say what causes it. I'm running a modified version with additional debug code and have to wait till it occurs since I have no idea how to debug this but I'm on it :+1:

cmtjk commented 4 years ago

@dkanada I was able to trace back the issue to AppManager.findApp(Intent intent). https://github.com/OpenLauncherTeam/openlauncher/blob/825fa18f04fe1e2e95e1526bc08d3ee5a69914c5/app/src/main/java/com/benny/openlauncher/util/AppManager.java#L61

It seems like the for-loop over _apps does not always return the correct result and therefore _apps might be inconsistent. Here's a full log where 4 of 6 items within one group where removed despite they were still installed and not hidden:

The following app was deleted:
> Group Item: com.benny.openlauncher.model.Item@6448817
> Intent: Intent { act=android.intent.action.MAIN flg=0x10000000 cmp=de.tutao.tutanota/.MainActivity }
> Component: ComponentInfo{de.tutao.tutanota/de.tutao.tutanota.MainActivity}
> Class Namede.tutao.tutanota.MainActivity
> Package Name: de.tutao.tutanota
Where:
> App is null

The following app was deleted:
> Group Item: com.benny.openlauncher.model.Item@c63bded
> Intent: Intent { act=android.intent.action.MAIN flg=0x10000000 cmp=org.thunderdog.challegram/.MainActivity }
> Component: ComponentInfo{org.thunderdog.challegram/org.thunderdog.challegram.MainActivity}
> Class Nameorg.thunderdog.challegram.MainActivity
> Package Name: org.thunderdog.challegram
Where:
> App is null

The following app was deleted:
> Group Item: com.benny.openlauncher.model.Item@5bc7c70
> Intent: Intent { act=android.intent.action.MAIN flg=0x10000000 cmp=org.kman.AquaMail/.ui.AccountListActivity }
> Component: ComponentInfo{org.kman.AquaMail/org.kman.AquaMail.ui.AccountListActivity}
> Class Nameorg.kman.AquaMail.ui.AccountListActivity
> Package Name: org.kman.AquaMail
Where:
> App is null

The following app was deleted:
> Group Item: com.benny.openlauncher.model.Item@dcb4be9
> Intent: Intent { act=android.intent.action.MAIN flg=0x10000000 cmp=com.moez.QKSMS/.feature.main.MainActivity }
> Component: ComponentInfo{com.moez.QKSMS/com.moez.QKSMS.feature.main.MainActivity}
> Class Namecom.moez.QKSMS.feature.main.MainActivity
> Package Name: com.moez.QKSMS
Where:
> App is null

Looking at the rest of the class I think there's a race condition with AsyncGetApps which modifies _apps. One solution would be to introduce locks on AsyncGetApps.execute() or to move _apps.clear() https://github.com/OpenLauncherTeam/openlauncher/blob/825fa18f04fe1e2e95e1526bc08d3ee5a69914c5/app/src/main/java/com/benny/openlauncher/util/AppManager.java#L160 several lines down to reduce the time it's empty.

cmtjk commented 4 years ago

requires https://github.com/OpenLauncherTeam/openlauncher/pull/580 to fix mentioned race condition

dkanada commented 4 years ago

So both are fine now? I can review these sometime this week.

the AppUpdateReceiver doesn't work for the work profile apps

I didn't realize this earlier, but I commented the cause of the uninstall issues earlier in the thread that is causing app drawer issues for certain apps.

cmtjk commented 4 years ago

Yes, I think so. The issue is hard to reproduce since you have to run the AsyncTask while the groups are checked/updated. But the changes made in https://github.com/OpenLauncherTeam/openlauncher/pull/580 seem reasonable.