Skip to content

feat(imex): Cancel BackupProvider when dropped#4242

Merged
flub merged 4 commits into
stable-1.112from
flub/backup-provider-drop
Mar 29, 2023
Merged

feat(imex): Cancel BackupProvider when dropped#4242
flub merged 4 commits into
stable-1.112from
flub/backup-provider-drop

Conversation

@flub

@flub flub commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

This ensures that the BackupProvider will be stopped as soon as the struct is dropped and the imex progress error event is emitted. This makes it easier to use and also makes sure that the ffi call dc_backup_provider_unref() does not lead to dangling resources.

There were some alternatives to doing this, but I wanted to make sure dropping before it is finished also results in a Imex(0) aka SendProgress::Failed event.

Closes #4179

This ensures that the BackupProvider will be stopped as soon as the
struct is dropped and the imex progress error event is emitted.  This
makes it easier to use and also makes sure that the ffi call
dc_backup_provider_unref() does not lead to dangling resources.
@flub flub requested review from dignifiedquire, link2xt and r10s March 28, 2023 16:48
@flub

flub commented Mar 28, 2023

Copy link
Copy Markdown
Contributor Author

@r10s would be great if you could confirm this behaviour meets what you'd expect from the FFI API.

@flub flub mentioned this pull request Mar 28, 2023
25 tasks
@r10s

r10s commented Mar 29, 2023

Copy link
Copy Markdown
Contributor

@r10s would be great if you could confirm this behaviour meets what you'd expect from the FFI API.

the additional error-event-on-abort is sth. the UI has to take care of - however, UIs have to be take care when accessing objects on events anyway. shutdown is hard :)

so, i'd say, it is good. UIs can also make use of error-event-on-abort as there seems is a guarantee about either success or error event at the end now. also, it is properly documented.

however, if the error-event-on-abort was not there before, we should consider to classify this as an API-change in changelog.

@flub

flub commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

however, if the error-event-on-abort was not there before, we should consider to classify this as an API-change in changelog.

There was already always an event, but it was wrongly the imex 1000 event (success) instead of imex 0 (failure). I've clarified this in the changelog.

@flub flub merged commit 943c8a1 into stable-1.112 Mar 29, 2023
@flub flub deleted the flub/backup-provider-drop branch March 29, 2023 12:51
@r10s

r10s commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

thanks!

r10s added a commit to deltachat/deltachat-android that referenced this pull request Mar 30, 2023
DcBackupProvider.unref() will soon also emit errors,
see chatmail/core#4242 ,
but also before, this was only correct by coincidence
r10s added a commit to deltachat/deltachat-android that referenced this pull request Apr 1, 2023
DcBackupProvider.unref() will soon also emit errors,
see chatmail/core#4242 ,
but also before, this was only correct by coincidence
r10s added a commit to deltachat/deltachat-android that referenced this pull request Apr 2, 2023
DcBackupProvider.unref() will soon also emit errors,
see chatmail/core#4242 ,
but also before, this was only correct by coincidence
r10s added a commit to deltachat/deltachat-android that referenced this pull request Apr 2, 2023
DcBackupProvider.unref() will soon also emit errors,
see chatmail/core#4242 ,
but also before, this was only correct by coincidence
r10s added a commit to deltachat/deltachat-android that referenced this pull request Apr 4, 2023
* make sure, only one error alert is shown

DcBackupProvider.unref() will soon also emit errors,
see chatmail/core#4242 ,
but also before, this was only correct by coincidence

* do not handle events on shutdown

DcBackupProvider.unref() may emit events,
but that may happen also for other reasons.

be explicit by a flag,
relying on activity.isFinishing() or by the already removed observer seems
error prone, esp. on future changes. do not make me think :)
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