OpenLauncherTeam / openlauncher

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

Fix several bugs related to item position values of group items #552

Closed cmtjk closed 4 years ago

cmtjk commented 4 years ago

There's some guessing here when explaining the cause of the issues because I'm not really familiar with the implementation. Don't beat me.

Rational

The overall handling of group items seems somewhat error-prone and results in several issues like https://github.com/OpenLauncherTeam/openlauncher/pull/551. This PR fixes a problem with removing and renaming items within a group (the captures don't clearly show the main point: when dragging, instead of removing the item and placing it on outside of the group, it's placed again within the group).

NullPointerException when removing an item

remove_withingroup This is due to the _location of an item is sometimes not set properly. When dragging it directly from the drawer and placing it in the group, _location of the item is null. When dragging an group item and dropping it directly in the group again, _location is null. The flawed group state described in https://github.com/OpenLauncherTeam/openlauncher/pull/551 can also lead to items, where the _location is null. This is problematic when you want to remove an item, because the _location comparison in https://github.com/OpenLauncherTeam/openlauncher/blob/fd31da6e349101f3fc7755955fbf33ef8eabea15/app/src/main/java/com/benny/openlauncher/activity/homeparts/HpItemOption.java#L41 throws a NullPointerException.

Unfortunately, if _location is set correctly, https://github.com/OpenLauncherTeam/openlauncher/blob/fd31da6e349101f3fc7755955fbf33ef8eabea15/app/src/main/java/com/benny/openlauncher/activity/homeparts/HpItemOption.java#L44 will fail with a NullPointerException. Same goes for renaming items.

Deleting first item 'deletes'* the whole group

* not really, but it removes it till you restart OpenLauncher removefirst_ingroup This seems due to the the first item and the group entity share the same _x-, _y value. When deleting, the routine removes the item correctly on the database layer but entirely from the desktop till it's redraw (e.g. restarting OpenLauncher).

This PR introduces ItemPosition.Group do distinguish between items, which are not problematic to delete or rename with the current implementation, and items, which are error-prone in this case. Items which are part of a group will have _location set to ItemPosition.Group by default (to circumvent null values). Since removing an item from within a group causes a lot of errors, this is now prohibited: You must first remove the item from the group before removing it (this is not the best solution, but the simplest). Renaming an item will just skip the remove-and-add part. This seems to work here.

There are still some visual glitches under rare circumstances. I'll fix them, if I encounter one :fireworks: .

cmtjk commented 4 years ago

I'll need to test this one first. Did you see a way to fix the issue without having to remove the item from the group first?

Yes but I'll create a new PR for this since it's a new functionality which might introduce new bugs and this PR fixes issues. Unfortunately, here are a few more things to consider when deleting items from within a group (like disbanding the group afterwards if only one item is left, etc) and I'm not quite sure if the existing code allows for a clean solution.

The functionality of removing an item from within a group will replace to current Toast message as soon as this PR gets accepted and I finished creating a clean solution.

I'm currently running a build including this PR on my phone for testing.

dkanada commented 4 years ago

The second most requested feature (behind custom icons) is positioning items within a group, so we might want to use the position attribute in the future to accomplish that.