Migrate to new group id

[Altonss]

A lot is still missing and the code hasn't been tested yet, it's work in progress :)
This PR solves #1045

[Altonss]

By the way @TheLastProject, do you think the methods of dbhelper that where using the group name (legacy id) should be switched to the group id? Or should we keep the compatibility with all the places where these methods are used?

And also, where should the constraint be coded, that the name has also to be unique?

[TheLastProject]

I think we should switch everything to use the group ID instead of the group name. It may be more work now, but it makes the code base simpler to only have one method of getting the group.

For the constraint, I'd say we just put that in the SQLite. Then we don't have to bother with writing Java code for it, we can just catch the error when a group that already exists is being created and show the user a decent message (like the "needs to be unique" we have right now): https://www.sqlitetutorial.net/sqlite-unique-constraint/

[Altonss]

I think we should switch everything to use the group ID instead of the group name. It may be more work now, but it makes the code base simpler to only have one method of getting the group.

For the constraint, I'd say we just put that in the SQLite. Then we don't have to bother with writing Java code for it, we can just catch the error when a group that already exists is being created and show the user a decent message (like the "needs to be unique" we have right now): https://www.sqlitetutorial.net/sqlite-unique-constraint/

Thanks for the help! I will take the time to implement all in a clean way :)

[Altonss]

Should the hashcode method in Group.java be deleted? Now that we have the id, it doesn't seem usefull anymore...

[Altonss]

I don't know how to update ManageGroupActivity and ManageGroupsActivity to the new workflow with the id... Has someone an idea on how to handle this?

[TheLastProject]

Should the hashcode method in Group.java be deleted? Now that we have the id, it doesn't seem usefull anymore...

Looks like it could be removed on first glance yeah.

I don't know how to update ManageGroupActivity and ManageGroupsActivity to the new workflow with the id... Has someone an idea on how to handle this?

What exactly are you struggling with?

[Altonss]

What exactly are you struggling with?

I found a workaround :) I don't think it is particularly pretty, but we will see during the review :)
Now I only need to fix so unit tests that are still failing!

[Altonss]

Oh now I think I see why some tests are not passing!
I need to implement a new Catima importer with the new group id structure: but this needs to be handled correctly to not break catima exports realised with an older Catima version...

[Altonss]

Hey @TheLastProject I tried fixing everything, but 9 errors are remaining on the unit test. Despite trying to debug the remaining issue, I could not find the specific spot. Do you maybe know where the issue comes from?

[TheLastProject]

This is how I understand the test failures. Hacktoberfest is keeping me super busy so I don't really have the time to look into this super deeply right now, hope it still helps to find the issues.

[Altonss]

Hacktoberfest is keeping me super busy so I don't really have the time to look into this super deeply right now, hope it still helps to find the issues.

Don't worry, I totally understand! It's amaizing to see all this activity on Catima ^^ You're review is already helping me a lot!

[TheLastProject]

I really appreciate all your help. Would you mind if I review this after October ends? I've had so little time for myself in the evenings lately with Hacktoberfest that I'd really like to try to spread out the load a bit 😅

[Altonss]

I really appreciate all your help. Would you mind if I review this after October ends? I've had so little time for myself in the evenings lately with Hacktoberfest that I'd really like to try to spread out the load a bit sweat_smile

Sure take your time and enjoy your free time doing something else :) I don't really care about hacktoberfest labeling myself, so there is no rush! I'm just really happy and relieved, that I managed to code this whole migration (with your help) to a working state and hopefully will enable my group widgets PR :)
Anyway I'm really excited to read your review, but again take the time you need, we are all working on this project on our free time and it needs to remain pleasant and not a burden ❤️

New review

[TheLastProject]

Definitely a good move in the right direction but there are still some pieces of code I am worried about.

I also noticed that all the unit tests now only ever retrieve the group by name, while grabbing the group by ID is the canonical way to get a group and is much more important to test than the name. In fact, do we ever need to get the group by name? I think we only need to check if the name is already taken.

[Altonss]

Definitely a good move in the right direction but there are still some pieces of code I am worried about.

I also noticed that all the unit tests now only ever retrieve the group by name, while grabbing the group by ID is the canonical way to get a group and is much more important to test than the name. In fact, do we ever need to get the group by name? I think we only need to check if the name is already taken.

Thanks for all your comments and change requests, which all make sense! Unfortunately I don't have much time currently, so I will hopefully be able to review and fix them later :)

[Altonss]

Hello @TheLastProject ,
I resolved all I could and have left some comments on the other points that still need to be addressed (but where I need your opinion/advice) :)

[TheLastProject]

Thank you for still working on this, hope these notes help, going to make a new full review after that. At some point, I'm probably just going to have to bite the bullet. It's always extremely scary for me to make changes that touch the database, but it's going to be necessary to fix the shortcuts and Gadgetbridge support

[Altonss]

Thank you for still working on this, hope these notes help, going to make a new full review after that. At some point, I'm probably just going to have to bite the bullet. It's always extremely scary for me to make changes that touch the database, but it's going to be necessary to fix the shortcuts and Gadgetbridge support

Don't worry we will take all the time we need :) It shouldn't be a source of stress at all!

[Altonss]

Corrected the last change you requested, hope this helps you for the next full review :)

[TheLastProject]

Hi, could you please rebase this? That way the versionCode is the same as the main branch and I can more easily test the migration (because otherwise switching to this branch would be a downgrade forcing me to remove all data).

I'm going to try to dedicate some time to testing this soon. I do wonder though, looking at the code, I think there is nothing that migrates the old launcher and "device control" shortcuts people already made?

[Altonss]

Hi, could you please rebase this?

What would be the best way to do it in a "clean" way?

I think there is nothing that migrates the old launcher and "device control" shortcuts people already made?

I didn't integrate any specific migration code for this thing, but if I remember correctly old shortcuts still worked. But this needs to be tested properly.

[TheLastProject]

Hi, could you please rebase this?

What would be the best way to do it in a "clean" way?

Normally I would use git rebase but given the git commit history isn't really clean and I'll probably squash this anyway it probably doesn't matter much. If you don't want to risk things like this given how much work you already put in this I can do it for you.

I think there is nothing that migrates the old launcher and "device control" shortcuts people already made?

I didn't integrate any specific migration code for this thing, but if I remember correctly old shortcuts still worked. But this needs to be tested properly.

Interesting. I'll have to take a look at this yeah :)

[Altonss]

Normally I would use git rebase but given the git commit history isn't really clean and I'll probably squash this anyway it probably doesn't matter much. If you don't want to risk things like this given how much work you already put in this I can do it for you.

I just did a rebase, opened up a new PR (#1214), as it refuses to push to this one after the rebase. Therefor I will close this one for now.