feat: added record editing and deletion functionality #5
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds record editing and deletion functionality to the Tagius time tracking application. The implementation follows the TimeTagger API convention of marking records as "HIDDEN" for deletion and allows users to edit record descriptions through a bottom sheet interface.
- Introduces an
EditRecordBottomSheetthat allows users to edit or delete time records - Adds
updateRecordanddeleteRecordmethods to the repository and view model layers - Implements item click handling in the records adapter to open the edit bottom sheet
- Creates delete confirmation dialog for user safety
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/layout/fragment_edit_record.xml | New layout for the edit record bottom sheet with save/delete buttons |
| app/src/main/res/drawable/ic_delete.xml | New delete icon drawable resource |
| app/src/main/java/net/ardevd/tagius/features/records/viewmodel/RecordsViewModel.kt | Added updateRecord and deleteRecord methods to handle record modifications |
| app/src/main/java/net/ardevd/tagius/features/records/ui/list/RecordsFragment.kt | Updated adapter initialization to include item click handler for opening edit sheet |
| app/src/main/java/net/ardevd/tagius/features/records/ui/list/RecordsAdapter.kt | Added onItemClick callback and wired up click listeners on record items |
| app/src/main/java/net/ardevd/tagius/features/records/ui/list/OnRecordsClickListener.kt | New interface file (appears unused) |
| app/src/main/java/net/ardevd/tagius/features/records/ui/edit/EditRecordBottomSheet.kt | New bottom sheet fragment for editing and deleting records |
| app/src/main/java/net/ardevd/tagius/features/records/data/RecordsRepository.kt | Added updateRecord and deleteRecord repository methods implementing TimeTagger API conventions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/main/java/net/ardevd/tagius/features/records/ui/edit/EditRecordBottomSheet.kt
Outdated
Show resolved
Hide resolved
| return try { | ||
| val response = apiService.updateRecords(listOf(deletedRecord)) | ||
| response.accepted.contains(record.key) | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
Exception is silently swallowed without logging. For consistency with other methods in this repository (e.g., startRecord at line 33 uses e.printStackTrace(), and fetchRecords at line 105 uses Log.d), consider adding error logging here to aid debugging.
| } catch (e: Exception) { | |
| } catch (e: Exception) { | |
| Log.d("Tagius", "Error deleting record: ${e.message}", e) |
| return try { | ||
| val response = apiService.updateRecords(listOf(updatedRecord)) | ||
| response.accepted.contains(record.key) | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
Exception is silently swallowed without logging. For consistency with other methods in this repository (e.g., startRecord at line 33 uses e.printStackTrace(), and fetchRecords at line 105 uses Log.d), consider adding error logging here to aid debugging.
| } catch (e: Exception) { | |
| } catch (e: Exception) { | |
| Log.d("Tagius", "Error updating record: ${e.message}") |
| class EditRecordBottomSheet( | ||
| private val record: TimeTaggerRecord, | ||
| private val onSave: (String) -> Unit, | ||
| private val onDelete: () -> Unit | ||
| ) : BottomSheetDialogFragment() { |
There was a problem hiding this comment.
Passing record and callback functions via constructor parameters will cause issues on configuration changes (e.g., screen rotation). Android will attempt to recreate the fragment using a no-arg constructor, which will fail. Consider using a companion object factory method with arguments bundle, or make TimeTaggerRecord implement Parcelable and pass it via arguments. See similar pattern in Android best practices for fragment instantiation.
app/src/main/java/net/ardevd/tagius/features/records/ui/list/OnRecordsClickListener.kt
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,5 @@ | |||
| package net.ardevd.tagius.features.records.ui.list | |||
|
|
|||
| interface OnRecordClickListener { | |||
There was a problem hiding this comment.
The interface name OnRecordClickListener doesn't match the file name OnRecordsClickListener.kt (note "Record" vs "Records"). They should match for consistency.
| interface OnRecordClickListener { | |
| interface OnRecordsClickListener { |
| android:layout_height="wrap_content" | ||
| android:layout_marginHorizontal="24dp" | ||
| android:layout_marginTop="24dp" | ||
| app:layout_constraintTop_toBottomOf="@id/title"> |
There was a problem hiding this comment.
The TextInputLayout is missing an android:hint attribute. For consistency with fragment_add_record.xml and better user experience, consider adding a hint like android:hint="Description" to guide users on what to enter.
| app:layout_constraintTop_toBottomOf="@id/title"> | |
| app:layout_constraintTop_toBottomOf="@id/title" | |
| android:hint="Description"> |
Fixes #4
Fixes #3