Show subtasks on details view. #442

[lemonboston]

This is far from finalized but I submit it now to get the discussion going.

Questions, notes:

Will using Tasks.PARENT_ID be okay for the subtask query?

UI: How should the subtask list look like? Same view repeated, with only the first having the icon, but same spaces between them as with other task details? Or should subtasks be listed within a 'frame' with maybe less vertical gap between them? Use some actual frame lines?
(Current state is just a quick initial setup, not intended to stay like this)

About the loading using RxJava2, I guess there can be many questions there, please check the code and let me know.

I haven't given much thought about packaging the new classes yet, so that's left as well.

Important thing about the loading: currently it is started in onLoadFinished() because when it was in onCreateView(), the view items didn't show up after loading the screen the second time. Not sure what that was, maybe it's only because SubtaskView currently adds the items to a LinearLayout and calls requestLayout() after it, all within a ScrollView, so maybe if that happens before the original content is loaded, then it doesn't work for some reason.

Please check the loading multiple times, it varies greatly for me, sometimes immediate, sometimes delayed compared to the original content.
So unfortunately this multiple loading does add some uncertainty here. As I mentioned before, I think one approach could be to do one loading, so zip/merge the 2 flows together and deliver the results once. That requires to 'Rxify' those loading, if we used Rx for the zip.

A little bit related that compared to current state where loading is handled in the fragment, I think it could actually be nicer to have a custom view for the subtasks (/properties) part that handles loading on its own, when let's say update(mTaskUri) is callled on it. But this would mean separate loading.

Please feel free to also comment on specifics already even though it is in initial state.

[lemonboston]

Shall we create subtasks master branch? To integrate the stories for this epic, like sorting out the relations as well, etc.

[dmfs]

How about using the list view row layout for the subtasks on the details view? Or something similar?
That would probably mean to omit the icon and replace it with a label "Subtasks".

[dmfs]

I created two mock-ups of this view (for some reason the fonts of the subtasks are not correct)

[dmfs]

Are you aware that the due date on the sub tasks is not correct? It shows Jan 1st 1970.
Also, please re-add the progress indication (the gradient). I know that my mocks don't have any but that's because I didn't find a way to do that in proto.io

[dmfs]

Why does it have to be a Single? Just make it an Optional.

[lemonboston]

You're right, that was totally unnecessary. Changed it.

[dmfs]

No, every task belongs to a list which has a color.

[lemonboston]

Thanks. I've updated the class to use Bolts Color and removed the TODO questions.

[dmfs]

Just import color-bolts via jitpack.

[lemonboston]

Ok, done, this is using Bolts Color now.

[dmfs]

This TaskView should actually be passed to a ctor. There could be a secondary ctor which creates a default TaskView though.

[lemonboston]

Done

[dmfs]

What's wrong with the ReferringTo version? I'd definitely prefer it over the EqArg solution. To create a RowReference from a Uri use RowUriReference.

[lemonboston]

I think I just didn't find that RowUriReference.. Updated now to use ReferringTo.

[dmfs]

The name of this class is definitely wrong. It's not a decorator so it must contain a noun.

[lemonboston]

I've changed the way this population is implemented (removed ListItems).

[dmfs]

This is the wrong idea. Please don't create new classes which work with Time. Instead add a format method to DateFormatter which converts from DateTime to Time internally and delegates to the original method (eventually we'll move the functionality over to the new method). This will help us to get rid of Time more quickly instead of spreading new traces of it all over our code.

[lemonboston]

You are right, it's better to keep those at a single place currently. I've changed this.

I plan to add tests to the conversion method. But since you know the topic much better, could you please help me by telling whether this looks good to you?
(I am not sure of what test cases are needed, so that will need review as well)

private Time toTime(DateTime dateTime)
{
	Time time = new Time();
	time.set(dateTime.getTimestamp());
	time.timezone = dateTime.getTimeZone().getID();
	time.allDay = dateTime.isAllDay();
	return time;
}

[dmfs]

I guess this is primarily supposed to be used as a decorator. Consider calling it just Darkened.

[lemonboston]

Right, it is a decorator, I've changed the name.
Also imported latest color-bolts with ValueColor and using that in the ctor now.

[dmfs]

This will crash if the dateTime has no time zone (I didn't have Optional back then) like it's the case for all-day tasks.

[lemonboston]

I spent more time now with this conversion, updated the conversion code and the test as well (which now passes). I assumed that we don't support non-all-day floating DateTime because we can't represent that with Time. So the conversion throws now in that case.

It is also not clear what is best way to set the alll-day flag on Time, directly or with a set(year, month, day) method, I left a TODO to check back on that.

Let me know if this DateFormatter update should be submitted with a separate ticket and pull request.

[dmfs]

This is why the progress bar doesn't appear in the details view, you just don't load it (so it's always returned as null). You also have to make sure to load TIMEZONE and IS_ALLDAY in order to get DUE right. That might explain the "Jan 1st 1970" issue, not sure.

[lemonboston]

Right, I've added them.

What do you think about instead of passing RowDataSnapshots directly to UI, using specific data/model types that the actual UI needs?
In general this may require creating many of these special, 'UI-local' data types but it decouples the UI from the internals of the data layer representations.

Something like this:

interface SubtasksUiData extends Iterable<SubtaskUiData>
{
      Color parentTaskColor();
}

interface SubtaskUiData
{
       CharSequence title();
       Optional<DateTime> due();
       Optional<Integer> percentComplete();
}

So SubtatskView could be SmartView<SubtasksUiData>,
SubtaskUiData would of course adapt RowDataSnapshot using the smaller adapters. There could be less potential for this kind of problem of missing columns from the projection.
(Maybe the small adapters could also expose the required projection in a constant? Not very elegant.. not sure what's the best way to handle that clearly.)

[dmfs]

This ctor should take a RowReference, everything else (Uri, RowSnapshot ) is subject to secondary ctors.

[lemonboston]

But we need authority and client and projection as well, right? So:

public Subtasks(@NonNull RowReference<Tasks> parentTask, @NonNull String authority, @NonNull ContentProviderClient client, @NonNull String... projection)

(I will probably create separate ticket for these taskspal related changes as well, later.)

[dmfs]

the primary ctor should only take a View<Tasks> and the RowReference<Tasks>.
Everything else (including creation of a View<Tasks> from authority, client and projection) is merely for convenience and should be done by secondary ctors.

[lemonboston]

Ok, I've update the constructor.

[dmfs]

Here is another question: does it make sense to add another method to RowSnapshotData called numberData(…) returning an Optional<Number>? This could make it easier in many cases.

[lemonboston]

I see now that Number has these intValue(), longValue(), etc, methods that we could use in the client code, but how could you implement returning those Number values in RowDataSnapshot correctly with general code? Not knowing which Number subtype to use.

[dmfs]

my idea was to extend RowSnapshotData with a method like this

@NonNull
Optional<Number> numberData(@NonNull String key);

A number numberData could then return an Optional like so

public Optional<Number> numberData(@NonNull String key)
{
   return new Mapped<CharSequence, Number>(chars -> new StringNumber(chars.toString()), charData(key));
}

…

public final static class StringNumber extends Number
{
   // for some weird reason Integer.parseInt(…) et al. expect a String when they would work just as well with a CharSequence :-|

    private final String mData;
    public StringNumber(String data) {mData = data;}

    public int intValue()
    {
       return Integer.parseInt(mData);
    }

   public float floatValue()
   {
      return Float.parseFloat(mData);
   }


   //etc …
}

So this could almost count as a convenience method but I think it can really improve our code.

[lemonboston]

Sounds nice to me. The type may need to be T extends Number I think, to be able to get an Optional<Integer> for example and use the the default value method like .value(3).

Create a ticket in ContentPal?

[lemonboston]

oh, that Optional<Integer> wouldn't work of course, since it's actually an Optional<StringNumber>... so I am not sure how to use a default value easily with Optional`

[dmfs]

How much efforts would it take to split this into multiple PRs? For instance, the progressbar smartview would make sense on its own. Same goes for the OpenTasksPal changes.

[lemonboston]

I agree that it would be better as multiple PRs. I will split it and create tickets for them.
The DateTime support on the formatter with the corresponding DateTime to Time conversion will also be one.

[dmfs]

@lemonboston please rebase.

[lemonboston]

I've rebased this one, but please note that there is #484 for the OpenTasksPal parts of this changeset and other parts may be submitted separately as well, so it is not worth reviewing this branch further at the moment.

[lemonboston]

I've updated this pull request with changes from master and also changed several other things.

Most notably, the two SmartViews (SubtasksView and SubtaskView) don't receive RowDataSnapshot any more but dedicatated Params objects, so that the UI is decoupled from the details of the data access layer this way. These Views no longer need to be concerned about projections for example.

The rx.Single that loads the required data - SubtasksViewParamsSource - now produces those ready made parameter objects. It is a special class for this purpose but uses re-usable components inside. It adapts the RowDataSnapshot to those Params before delivering them, so all those details, including the query as well, are hidden in this class.
I like this decoupling of data access layer representation details from the presentation layer.

Another important change I had to make was to move the loading from the ViewTaskFragment's onCreateView() to the updateView() method. This was because on rotation, the updateView() method is called with a delayed post() and it clears the whole content view, so the subtasks part was not shown. Not sure if there is a better option currently where to put it without starting to change those tangled code. I think the way forward could be to refactor the whole fragment, use rx for the loadings of ContentSet and Model so that everything is delivered together. I think any loading could be wrapped in RxJava sources without problems, the part that I haven't checked so far is the automatic updates. But I think that could be included, too. The Observable would just deliver the ContentSet-Model-SubtaskData the same way for initial loading and for updates as well. Most of those states and null checks involved could be removed.
So I think even without getting rid of ContentSet first, we could reduce complexities around loading and state with putting an Rx layer on top, sort of between the data-access and the UI.
(I'd volunteer for such a task. Maybe just a PoC first to see how it would work.)

One more note regarding the RxJava usage: From the four main source types of RxJava2 I used the SingleSource here, since we just have one result to emit. I started using that interface first for declarative classes, but to be able to use any operator on it, like map(), it has to 'wrapped' with the rx.Single class, calling Single.wrap(singleSource).map(..). So I ended up just using Single instead to avoid those boilerplates.
(Side note that in Kotlin I think this problem could be solved with extension functions on SingleSource.)

Back to the loading of the UI, the subtasks section now sometimes appears later, sometimes it is loaded quickly so that it appears to be appearing together with the rest. Because it varies I am not sure if we should try to add some animation, or how to smoothen it.
(Especially if we consider switching to unified rx loading, then it wouldn't be needed at that point.)

The branch is ready for review. I am looking forward to hear what you think.

[lemonboston]

Regarding the packaging of the new classes, I created a detailsscreen package for classes that are completely special to the details screen. ViewTaskFragment could come here as well if you see it fit, I just didn't want to make unnecessary changes here.

Classes in the new readdata package of the app are related to loading data from the provider using RxJava.

I was also able to remove TaskUri class, there was no need for it in that form.

[lemonboston]

I've rebased this and added commit which changes the subtask loading RxJava source class that it is no longer responsible to know about the views. The RowDataSnapshot -> View.Params adapter classes are now top level ones.

[mkampl]

Hi there, this feature would be handy for me and for an organization I am currently trying to manage the tasks via nextcloud. What is the status of merging this subtask feature?

[dmfs]

This PR needs to wait for recurrence support which has currently highest priority (after fixing #728). There may be a few required changes after recurrence support is done. I'l get to that whenever my schedule allows me to.