Add #504 feature request #543
Conversation
| package com.github.ashutoshgngwr.noice | ||
|
|
||
| object Constants { | ||
| const val SELECTED_PRESET_ID = "selectedPresetID" |
There was a problem hiding this comment.
Can we move this constant to WakeUpTimerFragment's companion? A separate object to declare constants seems unnecessary since only a single class is using the constant in question.
There was a problem hiding this comment.
Of course, that solution is much better.
|
@robercoding I had to force-push your local fork to remove the translation commit. You'll need to reset your local branch to your local fork's origin. PS, don't run the following if you have commits saved locally but missing in your fork. It will remove those commits but the changes will remain in your working tree. git fetch --all
git checkout master
git reset origin/masterPS, you can also add the faulty test-case you mentioned in the issue. |
|
Hey @ashutoshgngwr I did reset my local branch with my fork, and now it shows me that local files are in unstaged state. What should I do there? Do I commit them or? And yeah, I will add the test-case. |
|
If these unstaged files are the values (strings) files from the commit that I removed, you can discard them by using: If any file contains your changes, you should commit it. Discard it otherwise. |
fix(app) Remove selectedPresetID from Shared Prefs once user delete the preset. clean(app) Improve code by setting the management of SelectedPresetID on Shared Preferences in WakeUpTimerManager.kt test(app) Add tests for features "Load Saved Preset", "Load First Preset" in WakeUpTimerFragmentTest.kt
|
I have made some changes that I believe that are much better than before and then added the tests, tell me if you approve them or want some other changes ✌️. |
There was a problem hiding this comment.
This is good work. It requires a small change.
Suggestion: We can remove PresetFragment's dependency on WakeUpTimerManager. Instead of removing the preference from storage on deleting a preset, we can use Preset.findByID(...) in WakeUpTimerManager.getSharedPrefsSelectedPresetID(...) to check if it still exists.
I am still running the UI tests on my machine. I don't know if they work, but I'll fix them from my end if they don't. :)
app/src/main/java/com/github/ashutoshgngwr/noice/WakeUpTimerManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/ashutoshgngwr/noice/WakeUpTimerManager.kt
Outdated
Show resolved
Hide resolved
|
@robercoding Hey, I am in the middle of making changes. I'll make the ones I left in review as well. I'll request a review from you once I am done. |
|
@robercoding Can you review the last two commits? GitHub won't allow assigning PR authors as reviewers. 😅 |
|
Hey @ashutoshgngwr don't worry, just saw the notification to review but I was kinda late, changes seem pretty good and I see you added new stuff like some unit tests and improved code, so all good 😉✌️. |
Added requested feature #504
I haven't created any UI Tests because it was giving me that issue with other already implemented UI tests.