Skip to content

bugfix/main_selection#114

Merged
mshdabiola merged 3 commits into
developfrom
bugfix/main_selection
Jun 12, 2025
Merged

bugfix/main_selection#114
mshdabiola merged 3 commits into
developfrom
bugfix/main_selection

Conversation

@mshdabiola

@mshdabiola mshdabiola commented Jun 12, 2025

Copy link
Copy Markdown
Owner

PR Type

Enhancement, Bug fix


Description

  • Refactored item selection in main feature.

  • Updated Compose compiler configuration.

  • Improved build scripts.

  • Replaced boolean selection with Set.


Changes walkthrough 📝

Relevant files
Configuration changes
2 files
AndroidCompose.kt
Updated Android Compose configuration                                       
+4/-2     
compose_compiler_config.conf
Updated Compose compiler configuration                                     
+2/-1     
Enhancement
5 files
MainScreen.kt
Updated MainScreen to use Set for selection               
+8/-6     
MainState.kt
Added setOfSelected to MainState.Success                                 
+1/-0     
MainViewModel.kt
Updated MainViewModel to use Set for selection         
+20/-18 
TopbarAndDialog.kt
Updated NoteCard to handle selection state                             
+3/-2     
instuctions
Added gradle command for compose compiler metrics and reports
+5/-1     
Bug fix
1 files
NotePad.kt
Removed selected property from NotePad                                     
+0/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • This commit updates the Compose compiler configuration and makes minor adjustments to the build scripts.
    
    - In `instuctions`:
        - Added `./gradlew --rerun-tasks assembleFossReliantDebug -PenableComposeCompilerMetrics=true -PenableComposeCompilerReports=true` command.
    - In `build-logic/convention/src/main/kotlin/com/mshdabiola/app/AndroidCompose.kt`:
        - Commented out one of the lines that added `compose_compiler_config.conf` to `stabilityConfigurationFiles`.
    - In `compose_compiler_config.conf`:
        - Replaced `com.mshdabiola.modules.model.data.*` with `com.mshdabiola.model.*`.
        - Added `kotlin.collections.*` to the list of stable classes.
    …d.selected`
    
    This commit refactors how item selection is managed in the main feature. It replaces the `selected` boolean property in the `NotePad` model with a `setOfSelected` (a `Set<Long>`) in `MainState.Success`. This set stores the IDs of the selected items.
    
    **Key changes:**
    
    -   **`feature/main/MainState.kt`**:
        -   Added `setOfSelected: Set<Long> = emptySet()` to `MainState.Success` to track selected item IDs.
    -   **`feature/main/MainScreen.kt`**:
        -   Updated logic for `navigateToSelectLevel` to use `(mainState.value as MainState.Success).setOfSelected`.
        -   Modified `noOfSelected` to be derived from `success.setOfSelected.size`.
        -   Updated `isAllPin` to filter `notePads` based on whether their IDs are contained in `success.setOfSelected`.
        -   The `LazyVerticalStaggeredGrid` items now receive the `setOfSelected` and pass the selection status to `NoteCard` via a new `isSelect` parameter.
    -   **`feature/main/MainViewModel.kt`**:
        -   `onSelectCard(id: Long)`: Now updates `_mainState.value` by adding or removing the `id` from the `setOfSelected` in `MainState.Success`.
        -   `clearSelected()`: Resets `setOfSelected` to an empty set in `MainState.Success`.
        -   Functions like `setPin()`, `setAlarm()`, `deleteAlarm()`, `setAllColor()`, `setAllArchive()`, and `setAllDelete()` now use the `setOfSelected` from `getSuccess()` to determine which notes to operate on.
        -   `copyNote()`: Now retrieves the ID of the note to copy from `getSuccess().setOfSelected.first()`.
    -   **`feature/main/TopbarAndDialog.kt`**:
        -   `NoteCard` composable:
            -   Added a new `isSelect: Boolean = false` parameter.
            -   The border of the card is now determined by the `isSelect` parameter instead of `notePad.selected`.
            -   Removed `selected = true` from the `NoteCardPreview`.
    -   **`modules/model/NotePad.kt`**:
        -   Removed the `selected: Boolean` property from the `NotePad` data class.
    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Null Pointer

    The copyNote function retrieves a notepad using notepadRepository.getOneNotePad(id).first(). Ensure that id is always valid and that the repository call always returns a result to prevent a potential null pointer exception. Consider adding null checks or error handling.

    val id = getSuccess().setOfSelected.first()
    val notepads = notepadRepository.getOneNotePad(id).first()
    Efficiency

    Multiple functions in MainViewModel filter the notePads list based on selected IDs. This could be optimized by creating a helper function to perform this filtering.

        val selected = getSuccess().setOfSelected
        if (selected.contains(id)) {
            _mainState.value = getSuccess().copy(setOfSelected = selected - id)
        } else {
            _mainState.value = getSuccess().copy(setOfSelected = selected + id)
        }
    }
    
    fun clearSelected() {
        _mainState.value = getSuccess().copy(setOfSelected = emptySet())
    }
    
    fun setNoteType(noteType: NoteType) {
        _mainState.value = MainState.Success(noteType = noteType)
    }
    
    fun setPin() {
        val selected = getSuccess().setOfSelected
        val selectedNotepad =
            getSuccess().notePads.filter { selected.contains(it.id) }
    
        clearSelected()
    
        if (selectedNotepad.any { !it.isPin }) {
            val pinNotepad = selectedNotepad.map { it.copy(isPin = true) }
    
            viewModelScope.launch {
                notepadRepository.upsert(pinNotepad)
            }
        } else {
            val unPinNote = selectedNotepad.map { it.copy(isPin = false) }
    
            viewModelScope.launch {
                notepadRepository.upsert(unPinNote)
            }
        }
    }
    
    private fun setAlarm(time: Long, interval: Long?) {
        val setOfSelected = getSuccess().setOfSelected
        val selectedNotes =
            getSuccess().notePads.filter { setOfSelected.contains(it.id) }
    
        clearSelected()
        val notes = selectedNotes.map { it.copy(reminder = time, interval = interval ?: -1) }
    
        viewModelScope.launch {
            notepadRepository.upsert(notes)
        }
    
        viewModelScope.launch {
            notes.forEach {
                alarmManager.setAlarm(
                    time,
                    interval,
                    requestCode = it.id?.toInt() ?: -1,
                    title = it.title,
                    content = it.detail,
                    noteId = it.id ?: 0L,
                )
            }
        }
    }
    
    fun deleteAlarm() {
        val selected = getSuccess().setOfSelected
        val selectedNotes =
            getSuccess().notePads.filter { selected.contains(it.id) }
    
        clearSelected()
        val notes = selectedNotes.map { it.copy(reminder = -1, interval = -1) }
    
        viewModelScope.launch {
            notepadRepository.upsert(notes)
        }
    
        viewModelScope.launch {
            notes.forEach {
                alarmManager.deleteAlarm(it.id?.toInt() ?: 0)
            }
        }
    }
    
    fun setAllColor(colorId: Int) {
        val selected = getSuccess().setOfSelected
        val selectedNotes =
            getSuccess().notePads.filter { selected.contains(it.id) }
    
        clearSelected()
        val notes = selectedNotes.map { it.copy(color = colorId) }
    
        viewModelScope.launch {
            notepadRepository.upsert(notes)
        }
    }
    
    fun setAllArchive() {
        val selected = getSuccess().setOfSelected
        val selectedNotes =
            getSuccess().notePads.filter { selected.contains(it.id) }
    
        clearSelected()
        val notes = selectedNotes.map { it.copy(noteType = NoteType.ARCHIVE) }
    
        viewModelScope.launch {
            notepadRepository.upsert(notes)
        }
    }
    
    fun setAllDelete() {
        val selected = getSuccess().setOfSelected
        val selectedNotes =
            getSuccess().notePads.filter { selected.contains(it.id) }
    
        clearSelected()

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle empty selection in copy

    The copyNote function assumes that getSuccess().setOfSelected always contains at
    least one element. Handle the case where the set is empty to prevent a potential
    NoSuchElementException. Consider using firstOrNull() and providing alternative
    behavior if no element is selected.

    feature/main/src/main/kotlin/com/mshdabiola/main/MainViewModel.kt [299-305]

     fun copyNote() {
         viewModelScope.launch(Dispatchers.IO) {
    -        val id = getSuccess().setOfSelected.first()
    +        val id = getSuccess().setOfSelected.firstOrNull() ?: return@launch // Handle empty set
             val notepads = notepadRepository.getOneNotePad(id).first()
             if (notepads != null) {
                 val copy = notepads.copy(id = -1)
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly points out a potential NoSuchElementException if getSuccess().setOfSelected is empty. Handling this case with firstOrNull() and a suitable alternative improves the robustness of the copyNote function. This is a significant improvement.

    Medium
    General
    Verify delete operation

    The setAllDelete function modifies the noteType to NoteType.TRASH. Ensure that this
    action correctly reflects the intended deletion behavior. If a true deletion is
    needed, the notepadRepository.upsert might not be the appropriate method. Consider
    using a dedicated delete function if available in notepadRepository.

    feature/main/src/main/kotlin/com/mshdabiola/main/MainViewModel.kt [285-296]

     fun setAllDelete() {
         val selected = getSuccess().setOfSelected
    -    val selectedNotes =
    -        getSuccess().notePads.filter { selected.contains(it.id) }
    +    val selectedNotes = getSuccess().notePads.filter { selected.contains(it.id) }
         clearSelected()
    -    val notes = selectedNotes.map { it.copy(noteType = NoteType.TRASH) }
         viewModelScope.launch {
    -        notepadRepository.upsert(notes)
    +        notepadRepository.delete(selectedNotes) // Assuming a delete function exists
         }
     }
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion is valid in that using notepadRepository.delete might be more appropriate than notepadRepository.upsert for deleting notes. However, it depends on the actual implementation of notepadRepository. The score is moderate because it's a suggestion to verify existing functionality, not a critical bug fix.

    Medium
    Verify pin selection logic

    The setPin function filters notePads based on selected IDs. Verify that this
    filtering logic correctly identifies the notes to be pinned. Consider adding
    logging or assertions to ensure the expected notes are being processed.

    feature/main/src/main/kotlin/com/mshdabiola/main/MainViewModel.kt [192-212]

     fun setPin() {
         val selected = getSuccess().setOfSelected
    -    val selectedNotepad =
    -        getSuccess().notePads.filter { selected.contains(it.id) }
    +    val selectedNotepad = getSuccess().notePads.filter { selected.contains(it.id) }
    +    // Add logging or assertions here to verify selectedNotepad contains the expected notes
    +    clearSelected()
    +    if (selectedNotepad.any { !it.isPin }) {
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion is correct in that adding logging or assertions would improve the setPin function's reliability. However, it's not a critical bug fix, but rather a suggestion to enhance the code's maintainability and testability. The score reflects this moderate impact.

    Medium

    @mshdabiola

    Copy link
    Copy Markdown
    Owner Author

    /improve --pr_code_suggestions.commitable_code_suggestions=true

    Comment thread feature/main/src/main/kotlin/com/mshdabiola/main/MainViewModel.kt
    Comment thread feature/main/src/main/kotlin/com/mshdabiola/main/MainViewModel.kt Outdated
    Comment thread feature/main/src/main/kotlin/com/mshdabiola/main/MainViewModel.kt
    @github-actions

    github-actions Bot commented Jun 12, 2025

    Copy link
    Copy Markdown
    Contributor

    Test results

    27 tests  ±0   27 ✅ ±0   16s ⏱️ ±0s
    18 suites ±0    0 💤 ±0 
    18 files   ±0    0 ❌ ±0 

    Results for commit 14567cf. ± Comparison against base commit 9db04f8.

    ♻️ This comment has been updated with latest results.

    This commit renames the function `setAllDelete` to `setAllToTrash` in `MainViewModel.kt` for clarity.
    
    - In `MainViewModel.kt`:
        - Renamed `setAllDelete()` to `setAllToTrash()`.
    - In `MainScreen.kt`:
        - Updated the call from `mainViewModel::setAllDelete` to `mainViewModel::setAllToTrash` in the `onDelete` lambda.
    @codecov-commenter

    codecov-commenter commented Jun 12, 2025

    Copy link
    Copy Markdown

    Codecov Report

    ❌ Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.
    ✅ Project coverage is 0.88%. Comparing base (9db04f8) to head (14567cf).
    ⚠️ Report is 775 commits behind head on develop.

    Files with missing lines Patch % Lines
    ...c/main/kotlin/com/mshdabiola/main/MainViewModel.kt 0.00% 18 Missing ⚠️
    .../src/main/kotlin/com/mshdabiola/main/MainScreen.kt 0.00% 7 Missing ⚠️
    ...n/src/main/kotlin/com/mshdabiola/main/MainState.kt 0.00% 1 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##           develop    #114      +/-   ##
    ==========================================
    - Coverage     0.88%   0.88%   -0.01%     
    ==========================================
      Files          156     156              
      Lines         6868    6871       +3     
      Branches       540     541       +1     
    ==========================================
      Hits            61      61              
    - Misses        6794    6797       +3     
      Partials        13      13              

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    🚀 New features to boost your workflow:
    • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

    @mshdabiola mshdabiola merged commit ed86ba0 into develop Jun 12, 2025
    7 checks passed
    @mshdabiola mshdabiola deleted the bugfix/main_selection branch June 12, 2025 16:46
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants