plusonelabs / calendar-widget

An calender widget for your Android home screen.
Apache License 2.0
369 stars 126 forks source link

Adds tasks/TODOs support #310

Closed ekalin closed 5 years ago

ekalin commented 5 years ago

Fixes #308.

This is still somewhat basic, but functional, fetching tasks from Samsung Calendar or Dmfs OpenTasks, and it should be relatively straightforward to add support for other applications (as long as one knows how to fetch data from its content provider and set up an Intent to open the task in that application).

ekalin commented 5 years ago

You're right, there are some big changes. I was aware of that when I made them, but I disagree that 90% of what's changed is just restructuring, and that it's unnecessary. I do think that they are important, and adding the task support without them might be possible, but would leave the code hard to maintain.

I do agree that the names suck. :-( I couldn't come up with anything better, but I wasn't really satisfied with them.

I'll add some more comments in the review comments themselves.

ekalin commented 5 years ago

I'll make the changes, but I'm somewhat busy lately. It'll take a few days, but I have not forgotten this PR. :-)

yvolk commented 5 years ago

@ekalin Thank you. And then we will talk about Tasks :-)

ekalin commented 5 years ago

OK, I've undone the renaming of CalendarQueryResultsStorage, and removed the tests refactoring.

As a side effect, the new tests are also gone :-) But I can work on some tests if this is going to be accepted. As a matter of fact, most of the tests could be done as simple unit tests (the exeception would be one test including calendar events and tasks, for this an integration test is valid), but due to the lack of other unit tests I suppose this would not be a viable option, right?

I haven't squashed the commits yet. If that's OK I'd rather squash them as a last step before merging, but if you really want I can do that now.

yvolk commented 5 years ago

@ekalin In order to help us move further I continued cleaning of your change, fixed a couple of bugs and pushed the whole change as the single commit: https://github.com/andstatus/todoagenda/commit/64bac60dc054d8270f39b5be29c2d0e4300954fd Two things to fix are there yet:

  1. Add Tasks permission(s) to PermissionsUtil (I removed your change in the settings activity for unity)
  2. Fix probable bug: org.andstatus.todoagenda.prefs.InstanceSettings#getTaskSource is unused ???
ekalin commented 5 years ago

I see that you're storing calendar and tasks together in CalendarQueryResultsStorage. This might make it complicated in MockCalendarContentProvider if it were necessary to create a test that includes calendar and tasks, it would need to look at the uri to differentiate each kind.

That little hack in WidgetConfigurationActivity is necessary so that the "Request permission" button disappears from the settings page when the permission is granted. It's a little inelegant, but I think any good solution would require changing to the Support Library's PreferenceActivity.

Which brings me to your TODO #1: I'm not sure what you expect. It's likely that each Task application will require a different permission to read its tasks, so the permission cannot be in PermissionsUtil.

InstanceSettings#getTaskSource ended unused; I'll remove it.

yvolk commented 5 years ago

I see that you're storing calendar and tasks together in CalendarQueryResultsStorage. This might make it complicated in MockCalendarContentProvider if it were necessary to create a test that includes calendar and tasks, it would need to look at the uri to differentiate each kind.

That's right. We will need to differentiate each result by Event Provider, and we have them more than two already (Android Calendar, Open Tasks, Samsung tasks...)

But let's do this at the next step. Here we are only cleaning the code from unneeded changes and getting rid of easy to see bugs.

That little hack in WidgetConfigurationActivity is necessary so that the "Request permission" button disappears from the settings page when the permission is granted. It's a little inelegant, but I think any good solution would require changing to the Support Library's PreferenceActivity.

Exactly the same problems with Calendar permission are solved by usage of PermissionsUtil methods. "Little hack" for each button/permission is not only dirty: it doesn't cover all problems not having the permission. In different parts of the app, not in the setting only.

Which brings me to your TODO #1: I'm not sure what you expect. It's likely that each Task application will require a different permission to read its tasks, so the permission cannot be in PermissionsUtil.

Different permissions for different widgets (if you have more than one), This is why PermissionsUtil takes (should take) settings of concrete widget as an argument.

InstanceSettings#getTaskSource ended unused; I'll remove it.

It should be used, because TaskSource may be different for different widgets on the same device! Create two widgets with different task sources and see what you get! (This is why tests are needed...)

yvolk commented 5 years ago

@ekalin Please continue your changes with my commit, so we wouldn't need to always compare different branches and rebase on master...

ekalin commented 5 years ago

Funny you agree about separating calendar events and tasks in QueryResultStorage, because that's what my code did. But never mind.

I've fixed the problem with two widgets with different TaksSources.

As for the permissions issue, I'm afraid I don't understand what's your idea about what should be done.

yvolk commented 5 years ago

Funny you agree about separating calendar events and tasks in QueryResultStorage, because that's what my code did. But never mind.

Yes, we need the separation, but not via creation of a separate List with tasks' results... I will do this.

I've fixed the problem with two widgets with different TasksSources.

Thank you.

As for the permissions issue, I'm afraid I don't understand what's your idea about what should be done. I will fix this also and show you.

I think that at this stage we can merge this PR. I will do additional fixes as separate commits and let you know.