Skip to content
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

Support batch deletion #70

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

tuancoltech
Copy link
Collaborator

@tuancoltech tuancoltech commented Aug 4, 2024

Description:

Support batch deletion: #47

Screenshots:

Screenshot 2024-08-04 at 15 13 12 Screenshot 2024-08-04 at 15 13 27

@tuancoltech tuancoltech self-assigned this Aug 4, 2024
@tuancoltech tuancoltech force-pushed the feat/support_batch_deletion_v2 branch 4 times, most recently from a49cb91 to 5cecfff Compare August 4, 2024 15:06
@tuancoltech tuancoltech force-pushed the feat/support_batch_deletion_v2 branch 2 times, most recently from c4c3c8c to 6342290 Compare August 8, 2024 15:07
@tuancoltech tuancoltech force-pushed the feature/new_ui_design branch 2 times, most recently from 90410db to 8df7cee Compare September 9, 2024 05:52
@tuancoltech tuancoltech force-pushed the feat/support_batch_deletion_v2 branch 3 times, most recently from 4752e78 to c0ec632 Compare September 21, 2024 03:15
@tuancoltech tuancoltech force-pushed the feat/support_batch_deletion_v2 branch 2 times, most recently from 10c794a to c0d8998 Compare October 9, 2024 14:44
@shubertm shubertm self-requested a review October 22, 2024 11:03
Copy link
Member

@shubertm shubertm left a comment

Choose a reason for hiding this comment

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

SVID_20241022_213104_1.mp4
  • Long click must select the item

  • Back button should deselect all

  • Switch off screen after selection and all checked states are lost

SVID_20241023_153320_1.mp4
  • Ripple should have shape of the note item. Observe carefully

@tuancoltech tuancoltech changed the base branch from feature/new_ui_design to main October 25, 2024 05:38
@tuancoltech tuancoltech changed the title WIP: Support batch deletion Support batch deletion Oct 26, 2024
@tuancoltech
Copy link
Collaborator Author

Good work @tuancoltech

SVID_20241029_103736_1.mp4

  • While in selection mode, clicking a note should add it to selection not open it. Opening should happen out of selection mode.

@shubertm This has been fixed. Please help check again!

@@ -91,6 +91,13 @@ class NotesRepoHelper
return UserNoteProperties(title, description)
}

suspend fun deleteNote(notes: List<Note>): Unit =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
suspend fun deleteNote(notes: List<Note>): Unit =
suspend fun deleteNotes(notes: List<Note>): Unit =


private val selectedNoteCount by lazy { MutableLiveData<Int>() }
val observableSelectedNoteCount by lazy { selectedNoteCount }
val selectedNotedForDelete = mutableListOf<Note>()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val selectedNotedForDelete = mutableListOf<Note>()
val selectedNotesForDelete = mutableListOf<Note>()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shubertm Thanks. Please review again!

Copy link
Member

@shubertm shubertm left a comment

Choose a reason for hiding this comment

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

Please fix problem with exposing mutable variables wherever possible

@@ -206,16 +230,45 @@ class NotesListAdapter(
notifyDataSetChanged()
}

fun getNotes(): List<Note> {
fun getNotes(): MutableList<Note> {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need mutable list here? We expect this list to change only within this class to have single source of truth

var onItemLongPressed: ((pos: Int, note: Note) -> Unit)? = null
var onItemClicked: (() -> Unit)? = null

private val selectedNoteCount by lazy { MutableLiveData<Int>() }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private val selectedNoteCount by lazy { MutableLiveData<Int>() }
private val selectedNotesCount by lazy { MutableLiveData<Int>() }

var onItemClicked: (() -> Unit)? = null

private val selectedNoteCount by lazy { MutableLiveData<Int>() }
val observableSelectedNoteCount by lazy { selectedNoteCount }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val observableSelectedNoteCount by lazy { selectedNoteCount }
val observableSelectedNotesCount by lazy { selectedNoteCount }

viewModelScope.launch(iODispatcher) {
notes.value = textNotesRepo.read() + graphicNotesRepo.read() + voiceNotesRepo.read()
notes.value.let {
withContext(Dispatchers.Main) {
onSuccess(it.sortedByDescending { note -> note.resource?.modified })
notes.value = it.sortedByDescending { note -> note.resource?.modified }
onSuccess(notes.value.toMutableList())
Copy link
Member

Choose a reason for hiding this comment

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

No need to pass MutableList here. We should not be passing mutable variable to other object unless specified

note: Note,
onSuccess: () -> Unit,
notes: List<Note>,
onSuccess: (newList: MutableList<Note>) -> Unit,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onSuccess: (newList: MutableList<Note>) -> Unit,
onSuccess: (newList: List<Note>) -> Unit,

Copy link
Member

Choose a reason for hiding this comment

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

This is ok

withContext(Dispatchers.Main) {
onSuccess.invoke()
onSuccess.invoke([email protected]())
Copy link
Member

@shubertm shubertm Nov 14, 2024

Choose a reason for hiding this comment

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

But this is not needed, I mean [email protected]()

@shubertm
Copy link
Member

SVID_20241114_062853_1.mp4
  • Ripple goes below extra view on notes with previews. In other versions it is ok

@shubertm shubertm closed this Nov 15, 2024
@shubertm shubertm deleted the feat/support_batch_deletion_v2 branch November 15, 2024 16:53
@shubertm shubertm restored the feat/support_batch_deletion_v2 branch November 15, 2024 16:54
@shubertm shubertm reopened this Nov 15, 2024
@tuancoltech
Copy link
Collaborator Author

SVID_20241114_062853_1.mp4

  • Ripple goes below extra view on notes with previews. In other versions it is ok

@shubertm I haven't been clear what the issue is.
Do you mean, the ripple is partially below the graphic thumb, while it should be on top of it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants