-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
#102 hide subjects from untis #361
base: develop
Are you sure you want to change the base?
Conversation
do not close after selecting all; implement own SubjectElementPickerPreference and SubjectsElementDialog
# Conflicts: # app/src/main/java/com/sapuseven/untis/preferences/DataStorePreferences.kt # app/src/main/res/values-de/strings.xml # app/src/main/res/values/strings.xml
Todo: Do not automute in hidden lessons. |
This should be ready now @SapuSeven |
The artifact link has expired, can you please create an new one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work. I left some suggestions in the review comments - feel free to discuss.
For the future, it would be best to omit refactoring of code not relevant to the PR and submit a separate PR for these changes instead - this helps the code review process.
It's better to have multiple smaller PRs that address specific issues instead of one big one.
app/src/main/java/com/sapuseven/untis/activities/main/Drawer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/ui/preferences/ElementPickerPreference.kt
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/workers/NotificationSetupWorker.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/activities/MainActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/activities/MainActivity.kt
Outdated
Show resolved
Hide resolved
Thanks for your review. I will look into the suggested changes soon™. I have now learned to do smaller PRs, as I once had to review such "monsters" myself. I won't be doing anything like that in the future. :p |
+1 this is exactly what i was looking for! |
…to 102-hide-subjects-from-untis # Conflicts: # app/src/main/java/com/sapuseven/untis/activities/SettingsActivity.kt # app/src/main/java/com/sapuseven/untis/data/databases/LegacyUserDatabase.kt
I finally had the time to apply the suggestions :). If there is more to do, please let me know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some minor things.
About unifying the single/multiple stored timetable values - I'll work on that myself.
app/src/main/java/com/sapuseven/untis/ui/dialogs/ElementPickerDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/ui/dialogs/ElementPickerDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/ui/preferences/ElementPickerPreference.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/workers/AutoMuteSetupWorker.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/activities/SettingsActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/activities/SettingsActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/activities/MainActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/activities/MainActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/activities/MainActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/ui/dialogs/ElementPickerDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/sapuseven/untis/ui/dialogs/ElementPickerDialog.kt
Outdated
Show resolved
Hide resolved
Also I removed the search field. |
Fair :) |
@SapuSeven can you make sure that you resolve the merge conflicts here? You are more familiar with the WeekView compose |
No description provided.