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

Subtask aded in detailedView #931

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

Subtask aded in detailedView #931

wants to merge 6 commits into from

Conversation

Nik-Sch
Copy link

@Nik-Sch Nik-Sch commented Apr 7, 2020

As mentioned in #443, I made some changes to the PR #469 including quicker adding/checking/deleting of subtasks

@Nik-Sch Nik-Sch mentioned this pull request Apr 8, 2020
@dmfs
Copy link
Owner

dmfs commented Apr 11, 2020

First of all, thanks for your efforts. I appreciate that. I really like the idea of using quick add to add subtasks.
Here are a few thinks I'd like to get sorted before merging your changes

  • I've rebased the original subtasks PR on the current master, you might want to rebase again
  • I still want to apply a few more changes to the original PR, in particular support for subtasks should be specified by the task model, so we can support models which don't support subtasks
  • I want to add some unit tests for the opentaskspal stuff
  • I'm going to merge the original PR in smaller chunks

I hope to get this sorted over the holidays. In the meantime I have only a few comments on your PR.

@@ -77,9 +79,10 @@
/**
* The maximum time to add for the first time the "Task completed" info is shown.
*/
private final static int COMPLETION_DELAY_MAX = 1500; // ms
private final static int COMPLETION_DELAY_MAX = 1000; // ms
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to keep this delay a bit longer so new users will notice the confirmation. Please note that the actual delay gets shorter with every confirmation shown.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I did not notice that, that's fine

@NonNull RowReference<Tasks> parentTask)
{
super(new QueryRowSet<>(view, projection, new ReferringTo<>(Tasks.PARENT_ID, parentTask)));
super(new QueryRowSetSorting<>(view, projection, new ReferringTo<>(Tasks.PARENT_ID, parentTask), new Present(Tasks.STATUS + ", " + Tasks.TITLE)));
Copy link
Owner

Choose a reason for hiding this comment

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

The custom QueryRowSetSorting is not necessary. You can apply the Sorted decorator to view giving you the same result. Check this out:

 super(
    new QueryRowSet<>(
        new Sorted<>(Tasks.STATUS + ", " + Tasks.TITLE, view),
        projection,
        new ReferringTo<>(Tasks.PARENT_ID, parentTask)));

Copy link
Author

Choose a reason for hiding this comment

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

Uh, that's awesome, thank you, I will adjust these changes tomorrow

Gabor Keszthelyi and others added 6 commits April 11, 2020 23:46
@Nik-Sch
Copy link
Author

Nik-Sch commented Apr 12, 2020

I rebased from the original PR #469, included your suggestions and have a few further ideas/bugs:

  1. Sometimes the subtasks view is loaded twice (see image). I don't really grasp why this could happen, maybe you have an idea.
  2. Maybe the finished subtasks or even the percentage of subtasks should influence the parent's completed percentage, shouldn't it? Currently this happens with the markdown checkboxes, I don't know a good way to integrate both. Maybe one could average both, the "description percentages" and the "subtasks percentages" and if both are >0 average them for the parent's percentage and if only one is >0 take that one.
  3. Another idea I had was to show the amount of subtasks a task has in the ListView but I was unsure about how to integrate it without overloading the list

two subtasks

@dmfs
Copy link
Owner

dmfs commented Apr 12, 2020

Please see my comments regarding your points:

  1. That's one of the reasons why the original PR needs some more work
  2. I'd leave that to a separate story. This can easily get complicated. For instance, in project management or software development it's common that (sub-)tasks have different "weights" (e.g. story points).
  3. Let's leave that to a separate story as well. This might need some support from the provider, e.g. we count the number of subtasks upon insert, so we don't need to do that every time when we show a task. Once we drop ExpandableListView and switch to a RecyclerView we could consider allowing to expand subtasks in the list.

I'd also like to reconsider deleting subtasks when deleting the parent. This may not always be the best choice. It might be better to ask the user whether subtasks should be kept and converted to "top-level" tasks or if they should be deleted too.

@Nik-Sch
Copy link
Author

Nik-Sch commented Apr 14, 2020

Ok, all of your points sound reasonable. Is there something I could further do?

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