Skip to content

1285 repository full di migration part 1#1295

Merged
Futsch1 merged 25 commits intoFutsch1:mainfrom
ThomasKiljanczykDev:1285-repository-full-DI-migration-part-1
Mar 22, 2026
Merged

1285 repository full di migration part 1#1295
Futsch1 merged 25 commits intoFutsch1:mainfrom
ThomasKiljanczykDev:1285-repository-full-DI-migration-part-1

Conversation

@ThomasKiljanczykDev
Copy link
Copy Markdown
Contributor

@ThomasKiljanczykDev ThomasKiljanczykDev commented Mar 14, 2026

@Futsch1 this one continues the work on using DI to inject repositories everywhere their used.
I have split it into arbitrary parts to limit the scope of individual PRs

There are a handful of instrumented tests changes.
I have encountered some hurdles there and spent bulk of my work on resolving them.
This includes changes to the notification tray open/closes handling and dialog handling.
I've also reduced the attempts from the default 10 to 3, because otherwise it took eternity for the tests to finish if a lot of them were failing.

@Futsch1
Copy link
Copy Markdown
Owner

Futsch1 commented Mar 14, 2026

@ThomasKiljanczykDev
Hm, I don't quite know what to say. On the one hand I am very grateful for your help, but I have created #1294 yesterday with my first steps towards the DI domain. I know you told me to keep the scope small, but it eventually grew and included stuff like reminderContext, which is quite omnious in some parts of the app. Now I can think of either throwing it all away (since I certainly took some wrong turns, because I am still learning modern Kotlin) or try to merge... somehow.

We need to find a way on how to work together. I am not comfortable with letting you do all the work, because eventually I am responsible for the app and need to understand what it does and how it works.

So I suggest the following: You continue until a certain point and then stop and let me continue. I think it does not work to refactor in parallel.

Edit: Ok, I was able to rebase my branch on yours without issue... So maybe it does work after all. I will reconsider and see to it that we get both branches merged.

@ThomasKiljanczykDev
Copy link
Copy Markdown
Contributor Author

ThomasKiljanczykDev commented Mar 14, 2026

@Futsch1 your PR is smaller, so it has priority.

Removing the ReminderContext will be needed but it's omnipresent in the codebase, so your approach with the intermediate glue to make it work with Hilt at places is sound.

I could have made this PR as a draft and wait for the other to complete.

Comment thread app/src/main/java/com/futsch1/medtimer/Autostart.kt Outdated
@Futsch1
Copy link
Copy Markdown
Owner

Futsch1 commented Mar 17, 2026

@ThomasKiljanczykDev
I restarted the one failing test - let's see if it is a flaky one. As soon as all tests are passed and my PR #1296 is merged, yours is ready to follow.

@Futsch1
Copy link
Copy Markdown
Owner

Futsch1 commented Mar 17, 2026

@ThomasKiljanczykDev
The SDK 28 tablet tests have a systematic issue. You can also download the reports of the failed job, they contain screenshots, logs and UI dumps of the failed tests.

# Conflicts:
#	app/src/main/java/com/futsch1/medtimer/MainActivity.kt
#	app/src/main/java/com/futsch1/medtimer/overview/ReminderViewHolder.kt
#	app/src/main/java/com/futsch1/medtimer/reminders/ReminderProcessorBroadcastReceiver.kt
#	app/src/main/java/com/futsch1/medtimer/reminders/RemoteInputReceiver.kt
…t.repeatingReminders test due to its unreliability
Comment thread app/src/main/java/com/futsch1/medtimer/reminders/AlarmProcessor.kt Dismissed
Comment thread app/src/main/java/com/futsch1/medtimer/reminders/AlarmProcessor.kt Dismissed
@ThomasKiljanczykDev
Copy link
Copy Markdown
Contributor Author

@Futsch1 this PR should be ready for final review/approval.

Please re-run the failed checks if you can.

I have implemented some test reliability changes so the tablet (API 28) tests should not fail.
They passed in the previous pipeline run.

Copy link
Copy Markdown
Owner

@Futsch1 Futsch1 left a comment

Choose a reason for hiding this comment

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

Just a minor one regarding the test, accepting the TODO change would be ok for me now to get this PR merged. I will then add my next PR for preparing further DI work

Comment thread app/src/androidTest/java/com/futsch1/medtimer/NotificationTest.kt Outdated
@Futsch1 Futsch1 merged commit fa2d114 into Futsch1:main Mar 22, 2026
10 checks passed
@ThomasKiljanczykDev ThomasKiljanczykDev deleted the 1285-repository-full-DI-migration-part-1 branch March 22, 2026 11:10
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