Refactor to always replace task details fragments instead of updating… #630
Conversation
| Uri taskUri = ContentUris.withAppendedId(Tasks.getContentUri(mAuthority), selectTaskId); | ||
| // TODO For now we get the id of the task, not the instance, once we support recurrence we'll have to change that, use instance URI that time | ||
| Uri taskUri = ContentUris.withAppendedId(Tasks.getContentUri(mAuthority), (long) TaskFieldAdapters.TASK_ID.get(cursor)); | ||
| // TODO Should we use TASK_COLOR? (That's not in the projection.) |
There was a problem hiding this comment.
No, TASK_COLOR should be used as some sort of secondary color, like on a banner, badge or additional color bar.
There was a problem hiding this comment.
Thanks, good to know. I've updated the parameter names to refer to the task list color.
|
@dmfs I've made the color args mandatory for the Fragments as discussed on Slack. It allowed to remove classes, simplify the code. I've kept |
|
I've changed my mind about those 2 classes, not worth adding and maintaining them for a single |
|
On tablets it looks a bit strange that the details view is faded while the main app bar just changes instantly. For now it's ok but we should think of something to improve this later on. |
|
I like that the app bar stays with the color of the last task when you delete it, instead of going back to the primary color. |
|
Rotating on a small tablet still doesn't work properly. Steps to reproduce:
Expected result:
Actual result:
Similarly (and probably related)
Expected result:
Actual result:
|
| @@ -65,7 +67,7 @@ protected void onCreate(Bundle savedInstanceState) | |||
|
|
|||
| if (savedInstanceState == null) | |||
| { | |||
| ViewTaskFragment fragment = ViewTaskFragment.newInstance(getIntent().getData()); | |||
| ViewTaskFragment fragment = ViewTaskFragment.newInstance(getIntent().getData(), new PrimaryColor(this)); | |||
There was a problem hiding this comment.
How about passing this color to the activity as well? On my device I see the primary color briefly when opening a task, we could avoid that by passing the color to the ViewTaskActivity. Just make sure you tolerate the absence of the color in case it's called by an external app.
There was a problem hiding this comment.
I've added color as an optional extra to ViewTaskActivity.
|
Strange, I couldn't reproduce the first case you described, that works as expected for me. I had also tested this. I compared with master, I see it works correctly there, and that the selected task item is highlighted in portrait mode as well. I am going to look into this. |
|
Make sure you create multiple tasks and select one of the latter ones. |
|
I've pushed a branch named
|
041d5d3 to
f2d9d6c
Compare
|
I've found the bug for the task list item selection highlight - |
|
I've added two more changes today, to fix issues on small tablet: Note that some of the issues we've encountered are already present in master version. #642 should be partially fixed here. The behaviour for showing the details in portrait mode is still not perfect: Ready for check again. |
|
It's expected behavior that the list is shown when you select a new task in landscape mode and rotate to portrait mode. |
|
The latest version crashes for me when adding a task in single pane mode: |
|
crashes in two-pane mode as well |
| activity.startActivity(new Intent("android.intent.action.VIEW", mTaskUri)); | ||
| activity.startActivity( | ||
| new Intent(Intent.ACTION_VIEW, mTaskUri) | ||
| .putExtra(ViewTaskActivity.EXTRA_COLOR, TaskFieldAdapters.LIST_COLOR.get(mValues))); |
There was a problem hiding this comment.
This won't work and causes the aforementioned crash because mValues doesn't contain the list color at this point. If the task is new, mValues has been created from scratch and not been loaded from the database, so it doesn't contain the list color yet.
There was a problem hiding this comment.
There is a field called mListColor which holds the color of the currently selected list.
|
Another crash when deleting a task in two-pane mode |
|
I've fixed the first crash about the color. I couldn't reproduce the 2nd one when deleting a task. I tried to figure it out from the stacktrace what may cause that and applied a speculative fix. Ready for check again. |
|
This is still not working correctly. Try on a phone:
Expected behaviorThe list is still shown Actual behaviorThe just closed details view is shown |
|
I'll look into that. |
… them when new task is selected. #624 Potential fix for the mysterious bugs on certain emulators. Fix task list item selection highlight issue by changing the twopane flag from being a TaskListFragment extra to just being read from the bool resource. Fix issue on small tablet, rotating to portrait not opening up task again. Fix issue when deleting a task and rotating. Fix the case when selected item is deleted by swiping it out in two pane mode. Fix the case when previously select task was shown incorrectly. Fix crash related to color when a new task is created with the editor. Compare task uri in ViewTaskFragment.Callback#onTaskDeleted() as well just to be sure. Rename callback methods, move implementations next to each other. Speculative fix for the crash when a task was deleted.
b311605 to
329cf7e
Compare
… them when new task is selected. #624
@dmfs It took some time to get everything correctly working here but it looks good now for me.
There is a field in
TaskListActivitynow to save the last used color, so it can be used while the selected task is loaded. It is useful upon starting up the app and after rotation.EmptyTaskFragmentalso has a hint color now, so we don't change back the color to primary after a deletion but keep it as it was for that task.ViewTaskFragmentcan be further simplified after these changes, I've created a separate ticket for that: #628This pull request should fix #609 and #525, too.
I've created 3 new classes for reading arguments from
Bundlehere, in order to read the color argument. They help now, but the plan would be to remove and replace them once the full solution from boxed-bolts is available, since that approach is much better and more general.