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

FEAT: added alarm and timer export and import functionality #53

Merged
merged 16 commits into from
Feb 5, 2025

Conversation

ronniedroid
Copy link
Contributor

@ronniedroid ronniedroid commented Apr 2, 2024

What is it?

  • Bugfix
  • Feature
  • Codebase improvement

Description of the changes in your PR

  • Update strings
  • Added DataImporter and DataExporter helper classes
  • Added ExportDataDialog layout file and activity
  • Added Import data and Export data entries to the settings activity
  • In the settings activity, added functionality to trigger the added helper functions
  • In the helper functions, added functionality to get alarms and timers from db and export them and vise versa
  • Added toJson() method to the Alarm and Timer models

Acknowledgement

@ronniedroid
Copy link
Contributor Author

Hey,

So this is fully working and tested, it does export the alarms and import them correctly.
One Issue though, let's say you are importing a json array with three alarm objects, all three alarms will be correctly imported into the database, but the alarms view sometimes shows only the first item, sometimes it shows two items and sometimes it shows all imported items. However all the alarms show up on app restart.

I tried debugging this a little but I am not very good it this, here is what I tested:

  • Logging all the alarms from the db in the mainActivity/importAlarms if the AlarmsImported/importAlarms returns OK returns the correct number of alarms.
  • Logging all the alarms from the db in the alarmsFragment/setupAlarms returns the wrong number of alarms, and this number is the same as the number of alarms shown in the alarms tab.

I am guessing this has to do with main vs background threat or both activities are not using the same instant of the DBHelper.

Any help would be appreciated.

@naveensingh
Copy link
Member

I'll take a look at this later but in the future, please create a discussion or feature request first (because we may not want to implement everything that is suggested):

If you try fixing a bug or adding a new feature, make sure that it is already reported at the given repository and the report is open without label needs triage. If the given issue is closed or still has needs triage label, your pull request will be rejected. The only exception are critical bugs that we weren't able to classify yet.

https://github.com/FossifyOrg/General-Discussion?tab=readme-ov-file#contribution-rules-for-developers

@ronniedroid
Copy link
Contributor Author

I'll take a look at this later but in the future, please create a discussion or feature request first (because we may not want to implement everything that is suggested):

@naveensingh I am sorry, I did read the contribution guidelines or I would not have ticked it, but I just have missed that part and I am sorry for that.

I do change ROMs a lot and every time I have to setup all my alarms again (I have many) and this was a feature I really needed so hopefully you will consider it :-)

@ronniedroid
Copy link
Contributor Author

I moved the export and import to the settings and removed them from the menu, the exporter now exports both alarms and timers, the import is broken now, will fix it soon.

@ronniedroid ronniedroid changed the title FEAT: added alram export and import functionality FEAT: added alarm and timer export and import functionality (WIP) Apr 18, 2024
@ronniedroid ronniedroid changed the title FEAT: added alarm and timer export and import functionality (WIP) FEAT: added alarm and timer export and import functionality Apr 21, 2024
@ronniedroid
Copy link
Contributor Author

@naveensingh This might not be a priority, but I did complete this PR, now timers and alarms are exported and imported together and I have done my best to handle errors especially while importing, your review of this when you have time is greatly appreciated :-)

@Aga-C
Copy link
Member

Aga-C commented Apr 21, 2024

  1. Why have you created an Export and import section in Settings? In all other apps it's called Migrating, and the string for it is in Commons.
  2. Why there's so much empty space under Export data?

image

  1. To make it look more as in other apps, strings should rather be Export alarms and timers and Import alarms and timers.
  2. In other apps, we don't have a toast informing the user that export/import has been aborted. It only happens while pressing back on save, so the user is aware of what they have done.
  3. Can you make string Partial import due to wrong data more similar to other apps? In other apps, we have Importing some entries failed and Importing some events failed.

We should keep consistent behavior, looks and naming between all the apps. Also, where it's possible, we should use strings defined in Commons.

@ronniedroid
Copy link
Contributor Author

@Aga-C I fixed all of the above.

For the space, I had put a subtitle but it was not showing when building, so I just removed it and now there is no space.

I did not completely understand point 4, I did remove the toast messages for aborting, I just don't know what "pressing back on save" means.

I am sorry for the inconveniences, next time I will make sure a string is available in the commons or that it is consistent with the other apps.

BTW, alarms and timers are populated correctly in the app now when imported, no need to restart the app to see them.

@Aga-C
Copy link
Member

Aga-C commented Apr 21, 2024

It seems to work fine, but there are two edge cases where it behaves strange.

First issue - duplicating alarms

When you import the same alarms and timers as you currently have in the app, the import:

  • duplicates Alarm entries
  • doesn't add duplicated Timer entries

I think the behavior should be consistent in both tabs, either with duplicating or not. IMO, we shouldn't duplicate at all.

Second issue - overwriting timers

Duplicate checking in Timers while import shouldn't remove already edited timers. I did something like this:

  1. Created a timer.
  2. Exported data.
  3. Changed that timer's sound and time.
  4. Imported data.

Instead of creating a new entry with old data, it has overwritten the current entry. I assume you check it only by ID (I haven't checked the code, just testing from the user perspective), because when I created another timer with exactly the same name, time and sound, it hasn't overwritten it.

@ronniedroid
Copy link
Contributor Author

I will test this tomorrow and see what is going on. I hadn't thought about duplicates tbh, thank you for testing.

@ronniedroid
Copy link
Contributor Author

ronniedroid commented Apr 21, 2024

Ok, for the first problem, I think I got what the issue is, not very sure how to fix it yet.

first, I was not backing up the isEnabled state, and was always setting it to false, this made a problem in comparing to see the if alarm !in dbHelper.getAlarms(), that fixed, the other issue is, the dbHelper.insertAlarm(alarm) doesn't take the id from the imported data, it inserts it with a new id (generated automatically) which causes the alarms that have been imported to be inserted again and again.

the values of insertAlarm are generated here

    private fun fillAlarmContentValues(alarm: Alarm): ContentValues {
        return ContentValues().apply {
            put(COL_TIME_IN_MINUTES, alarm.timeInMinutes)
            put(COL_DAYS, alarm.days)
            put(COL_IS_ENABLED, alarm.isEnabled)
            put(COL_VIBRATE, alarm.vibrate)
            put(COL_SOUND_TITLE, alarm.soundTitle)
            put(COL_SOUND_URI, alarm.soundUri)
            put(COL_LABEL, alarm.label)
            put(COL_ONE_SHOT, alarm.oneShot)
        }
    }

What do you suggest we do about this? Make an insertFromBackup function in DBHelper?

For the second issue, I think this is causing it

    @Insert(onConflict = OnConflictStrategy.REPLACE)
    fun insertOrUpdateTimer(timer: Timer): Long

EDIT: I have forgotten to push the latest changes where I am checking if the entries are in the database already or not.

@Aga-C
Copy link
Member

Aga-C commented Apr 22, 2024

But if you will consider ID in alarm data, you will probably have the same issue in alarms as there's now in timers. I think it should be manually checked if an entry with the same fields already exists. People won't have an enormous amount of alarms, so it shouldn't be slow.

@ronniedroid
Copy link
Contributor Author

But if you will consider ID in alarm data, you will probably have the same issue in alarms as there's now in timers. I think it should be manually checked if an entry with the same fields already exists. People won't have an enormous amount of alarms, so it shouldn't be slow.

I already fixed the alarms by comparing every entry but the ID.

Same did not work for the timers, as the insertOrUpdate function is updating the timer with the same ID, will figure that out too.

@ronniedroid
Copy link
Contributor Author

@Aga-C

Ok so, it all is working now.

for the timers, I am comparing all entries but the ID, and if it does not exist, I am updating the ID of the inserted timers with existingTimers.last().id?.plus(1), so it would make a new timer and not replace an already existing one.

I am not sure if this is the best approach, so I will await your comments.

@Aga-C
Copy link
Member

Aga-C commented Apr 22, 2024

In timers, if you export two entries with the same name (but different time), only one of them is imported. You can test on a JSON I've exported:
Export alarms and timers_2024_04_22_07_36_54.json

Also, I've found another problem - if you have empty timers list, there's an exception and nothing gets imported.

@ronniedroid
Copy link
Contributor Author

ronniedroid commented Apr 29, 2024

@Aga-C fixed, the problem was that I was inserting new alarms with the id set to last timer in the list + 1, so now I am just making sure to set the id to 1 if no timers are available.

@Aga-C
Copy link
Member

Aga-C commented Apr 29, 2024

It seems to be working fine. The only thing that bothers me is that whenever there are duplicates, it says Importing some entries failed. It sounds like there was an error, when in fact there wasn't any.

In Calendar, it works like this:

  • When some entries were already in the app, but some were new, it displays Importing successful.
  • When all entries were already in the app, it displays No new items have been found.

@ronniedroid
Copy link
Contributor Author

I agree, will update soon.

@ronniedroid
Copy link
Contributor Author

and it is done.

@Aga-C
Copy link
Member

Aga-C commented Nov 22, 2024

Fixes #105.

@naveensingh naveensingh self-assigned this Jan 31, 2025
Copy link
Member

@naveensingh naveensingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ronniedroid! I’ve suggested some changes that you might want to work on if you are still around. If not, I’ll address them myself later.

app/src/main/res/layout/activity_settings.xml Outdated Show resolved Hide resolved
app/src/main/kotlin/org/fossify/clock/models/Timer.kt Outdated Show resolved Hide resolved
@ronniedroid
Copy link
Contributor Author

Hey @naveensingh I am sorry, I would have applied the changes myself but can't as I don't have much time at hand, and my current PC is very underpowered for android Studio 😅

@naveensingh
Copy link
Member

No worries, I'll do it.

- Replaced manual serialization/deserialization with kotlin serialization
- Removed unnecessary intent creation
- Fixed import/export with SAF on Android 8 & 9 (storage permission was being requested but was never added to the manifest)
User pressing back is not an error.
@naveensingh
Copy link
Member

naveensingh commented Feb 3, 2025

val alarms = dbHelper.getAlarms()
val timers = timerDb.getTimers()
if (alarms.isEmpty()) {
    toast(org.fossify.commons.R.string.no_entries_for_exporting)
} else ...

It doesn't export timers when there are no alarms, is it intended or an oversight?

EDIT: I fixed it.

@naveensingh
Copy link
Member

Thanks, both of you!

@naveensingh naveensingh merged commit 04cbd0b into FossifyOrg:master Feb 5, 2025
2 of 4 checks passed
@ronniedroid ronniedroid deleted the export_alarms branch February 5, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants