#277: Change the order and remove tasks assigned to the plan #334
Conversation
There was a problem hiding this comment.
Thanks! I was doing some testing I am getting random problems. It is something worth to look at.
First, when I had 2 items on list and they were duplicating each time I swapped it. And current state was saved, I was closing app going back and I was able to remove it item by item.
Then after I removed it, I started adding new items, I was able to add same items twice - what I think should be ok - and then when It happened, swapping worked on the list view, but when I go back to menu and open list again changes were not saved. It is random and strange so I will take a look at this later also.
|
|
||
| @Override | ||
| public boolean onItemMove(int fromPosition, int toPosition) { | ||
| if (fromPosition < toPosition) { |
There was a problem hiding this comment.
Same code is in steps list. It would be good to extract it so some common component like. ItemOrderUpdater with method swapOrder(list, from, to)
| @Inject | ||
| ToastUserNotifier toastUserNotifier; | ||
|
|
||
| PlanCreateTaskListRecyclerViewAdapter.TaskItemClickListener taskItemClickListener = |
There was a problem hiding this comment.
I wonder if that code could be extracted to separate class implementing this interface, only create instance here ( dependencies could be pass in constructor) or are there any Android limitations?
| for(int i = 0; i < taskListAdapter.getItemCount(); i++){ | ||
| PlanTaskTemplate planTaskItem = taskListAdapter.getPlanTaskTemplate(i); | ||
| if(i != planTaskItem.getOrder()) { | ||
| planTemplateRepository.removeTaskFromPlan(planId, planTaskItem.getId()); |
There was a problem hiding this comment.
The problem could be here, why removing and creating new task, instead of updating the order? Ids will change. And the items in adapter's list have old ids still. I think it should be updating or setting new list to adapter should be added here.
| planTemplateRepository.setTaskWithPlan(planId, planTaskItem.getTaskTemplateId(), i); | ||
| reordered = true; | ||
| } | ||
| if(!removedTask && reordered){ |
There was a problem hiding this comment.
I think this if(line 66) was going to be after for loop to notify user once after all changes.
Resolves #277 #315