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

Improve task list selection when creating a new task from widget #287

Merged
merged 2 commits into from
Jul 2, 2017

Conversation

korelstar
Copy link
Contributor

@korelstar korelstar commented Oct 12, 2016

I'm using single widgets for every task list. In such a use case, it would be very useful, if a newly created task (using the + sign on the top-right corner of the widget) would use automatically the corresponding task list from the widget. Currently, the last selected list (from the previous edit) is used, which is often not desired.

This pull request therefore introduces a filter for the TaskList list in EditTaskFragment. Now, only such tasklists are shown, that are activated in the widget on which the + button was pressed.

What do you think about this solution? I see it as work in progress and looking forward for feedback.

Tasks done

  • pre-select a suitable task list for new tasks with respect to the calling widget
  • only consider writable task lists
  • decide and implement a selection strategy for 2+ task lists

@dmfs
Copy link
Owner

dmfs commented Oct 12, 2016

I see how this is a useful enhancement for widgets that show only a single list. I'm not so sure about widgets that show multiple lists, though. In the latter case the "benefit/annoyance" factor is probably not high enough to justify this change. Some users might be annoyed by not being able to choose from the whole set of lists anymore (if you change your mind about the list you would have discard your changes and start over or store the task in the wrong list and go back to change the list).

Here is a counter proposal, separated by the number of "writable" lists the widget shows:

0 writable lists

keep the current behavior

Reason: we're planning to add support for "smart lists" that show tasks that match certain criteria. These smart lists would be read-only by design as they are only "filtered views" on the set of tasks. Since these smart lists qualify perfectly for being shown in a widget (like "my tasks today") it would be counter-productive to not support adding tasks via the widget anymore.

1 writable list

Show all lists in the dropdown but pre-select the one that the widget shows, so you don't have to change the selection, but you still have the option.

In this case the benefit is exactly the same as in your PR - no need to change the list selection.

2+ writable lists

Like "1 writable list" but pre-select one of the writable lists of the widget. Then we just need to decide which of these lists to select. That could be:

  • the first one (ordered by _id)
  • the one with the most events (i.e. the one being used most frequently)
  • the one that has been used most recently
  • a random one

An approach like that could be done without any changes in the Editor code. The widget would just provision the Editor with a ContentSet that contains the pre-selected task list.

What do you think?

@korelstar
Copy link
Contributor Author

I think your considerations are reasonable. I thought about pre-selecting instead of filtering, too. But as you found out as well, this does not scale for widget with 2 or more lists. Therefore, I just tried the filter version. But I'm totally fine with switching to a pre-selecting approach. Let's do it this way.

So, the open question is which list should be selected for the "2+" case. I think, the most recently used task list would fit best (a random list would drive me crazy), if it can be computed with reasonable effort. How would you do that? I see two options:

  1. use the created time from the tasks in the database
  2. use separately saved time values for each list, similar to the PREFERENCE_LAST_LIST which is used currently (here, it would better to have a separate column in the task list table).
    I prefer the first one, if the created column is indexed.

Regarding the implementation: If the widget should provide the ContentSet to the Editor, then EditTaskActivity.EXTRA_DATA_CONTENT_SET should become public (see my new commit (I will squash them when all is done)).

@korelstar korelstar changed the title Widget: Filter task lists to lists of widget when creating a new task Improve task list selection when creating a new task from widget Oct 14, 2016
@korelstar
Copy link
Contributor Author

As far as I can see, there is no index on created for tasks: https://github.com/dmfs/opentasks-provider/blob/master/src/org/dmfs/provider/tasks/TaskDatabaseHelper.java#L617

This should be changed or another strategy should be chosen. What's your favorite?

@dmfs
Copy link
Owner

dmfs commented Oct 14, 2016

Using created could be quite confusing, because it would encompass tasks created on the serve or another client too (thinking of shared lists). So the selection might change on the client if you or someone else creates a task in another list on a another client. I think that makes it kind of unpredictable and defeats the purpose.

Since this is solely a UI feature we should do this entirely in UI code. So I'm in favor of your option 2.
Maintaining a list of timestamps of the last local insert for each list in the shared preferences shouldn't be difficult. Optionally we could just maintain an ordered set of list ids (sorted by the most recent insert).

Of course we could also add a column to the lists table, but we should make sure that sync adapter updates don't update the value of this column. So we can't just use an SQL trigger. Instead we have to do this in Java code, which makes it non-trivial.

@korelstar
Copy link
Contributor Author

korelstar commented Oct 15, 2016

OK, I understand. However, a disadvantage of the second option without using a column in the database is: if a task list is deleted, the corresponding entry in the shared preferences will not be deleted automatically.

So we will get orphaned data -- on a long-term basis. For a typical user, this will not be serious (How often do you delete a task list? I would implement this using a sorted set of ids, so 1 deleted task list would leave over ~2-4 Byte). But from time to time, this should be cleaned up. This could be done, e.g., on every change of this list or (the other extrema) only when the app is updated.

Or is there a trigger which is called when a task list was deleted?

@korelstar
Copy link
Contributor Author

I've just implemented the strategy "recently used task list" using SharedPreferences (see commit).

As far as I can see, EditTaskFragment.mSelectedList is not necessary anymore and could be removed.

I'm done so far. Please review, @dmfs

@dmfs
Copy link
Owner

dmfs commented Oct 24, 2016

Thank you! It looks good on a first glance. It's only the procedural approach of RecentlyUsedLists that I don't really like, but considering that we have to deal with the limitations of SharedPreferences it's probably ok for now.

I'll have a closer look later this week.

@korelstar korelstar changed the base branch from master to staging November 19, 2016 12:45
@korelstar
Copy link
Contributor Author

rebased to staging and ready to merge

@korelstar
Copy link
Contributor Author

Any news on this? Can this be merged, now? (after 7 months 😉 )

@dmfs
Copy link
Owner

dmfs commented Jun 29, 2017

Not sure why, but it doesn't work for me. I've created 4 lists and two widgets, showing 2 lists each. Whenever I click the ad button, the editor comes up with the last recently used list, even if the widget doesn't show that list.
I've tried with the small and the large widgets.
Is it possible that the editor fragment still overrides the selection?

@korelstar
Copy link
Contributor Author

Hmmm. It was that a long time ago.

It's fixed and tested again. Please test again and (hopefully) be able to merge now 😀

Copy link
Owner

@dmfs dmfs left a comment

Choose a reason for hiding this comment

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

work fine now, thx!

@dmfs dmfs merged commit 7029940 into dmfs:staging Jul 2, 2017
@korelstar korelstar deleted the widget-new-task branch July 4, 2017 18:55
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