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

[US 02.03.01] Select and delete selected items #45

Closed
wants to merge 4 commits into from

Conversation

Sami-Jagirdar
Copy link
Contributor

@Sami-Jagirdar Sami-Jagirdar commented Nov 6, 2023

Summary
Many changes made, but the gist of it is:

  • Edited the home fragment layout to include buttons depending on whether it is in select state or not

(Eg: It has both 'Select' and 'Cancel' buttons but only one of them is visible depending on whether you're in select state or not. Potential change is to make a separate layout for for when home is in select state, but this would also mean splitting the HomeFragment class into 2 classes, ideally 2 Activity classes with Intents to navigate between them).

  • Added a boolean attribute to class 'Item to indicate if it is selected or not
  • Similarly, modified ItemAdapter to indicate if the Item view is in select state or not. In select state, checkboxes can be ticked and doing do selects the corresponding item
  • in HomeFragment, implemented functionality to delete selected items from the database and update view

UML
To be added

select_delete_Apng
select_delete_Bpng

*Notes:

  • I added the delete and tags button in the select state on the top instead of the bottom nav bar as in the UI mockup. I did this because the bottom navbar is meant for navigating to different pages/activities, so doesn't make sense to add a button like delete here.
  • I have to refactor this code quite a bit, but will do it based on the comments and suggestions from other developers. Also, will add javadocs then.

}
// Every time the data list is updated, all the checkboxes are unselected
for (int i=0;i<itemList.size();i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things, first do we actually need to setChecked false? I believe the snapshot listener wipes the itemList with itemList.clear();. Not sure if that impacts the state in the list or not. Second, if we do really need this I am pretty sure we can avoid two for loops here by simply combining with the querySnapshots one as querySnapshots.size() = itemList.size()

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you could just call your unselectAllItems method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To your first comment, it is kind of necessary since the checkboxes are part of itemAdapter/itemView, not the Item class. So say you select an item meaning you selected a checkbox on its adapter, when you delete that item, the adapter still exists. The ArrayList also gets rearranged and some other item takes the previous item's position and it appears that it is selected because the checkbox is still ticked.

So either I move this for loop to unselectAllItems or I can make a single for loop in the ItemListener as you said. Lemme know which do you prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

I think sticking to the unselect method is probably best. Keep things consistent

if (itemList.get(i).isSelected()) {
itemRef
.document(itemList.get(i).getId())
.delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably look at finding a way to make this execute in a batch of some sorts. Like committing all the deletes at once. Cuz I think it's possible that after an item is deleted but before all items are deleted it could call the snapshot listener.

We could also just wrap them in a semaphore, but I think batching should be straightforward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep got it. I can use WriteBatch to do this easily. Will make the change soon.

}


// TODO: This is an interface method for a dialog fragment. Useless for now since I can't figure out how to have a dialog pop up inside a fragment
Copy link
Contributor

Choose a reason for hiding this comment

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

AddEditExpenseFragment.newInstance(expenseDataList.get(pos)).show(getSupportFragmentManager(), "EDIT_EXPENSE")) This is how I made a dialog fragment popup for expbook. Not sure if it helps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I assume the code you have here is inside your MainActivity file? Because I did do something similar in expbook too.
The issue here is that I implemented the dialog listener in HomeFragment and I get a runtime exception that MainActivity doesn't implement the listener interface. So I have to figure out a way to make it so that HomeFragment is the context for my dialog listener. I think jared did something like this where he made a callback. Will look into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, yep. That was in the mainactivity. Yep Callback interface is porbably a good idea

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

Successfully merging this pull request may close these issues.

2 participants