-
-
Notifications
You must be signed in to change notification settings - Fork 103
Use DAV:owner for "Show only personal"
#1646
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
base: main-ose
Are you sure you want to change the base?
Use DAV:owner for "Show only personal"
#1646
Conversation
Signed-off-by: Arnau Mora <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Updates the HomeSet initialization logic to properly determine if a home set is "personal" based on the DAV:owner property, addressing the issue where no home-set was ever marked as personal.
- Implements personal home set detection by comparing DAV:owner property with service principal URL
- Adds new
isPersonal()method to evaluate ownership and determine personal status - Updates home set creation to use the new personal detection logic
app/src/main/kotlin/at/bitfire/davdroid/servicedetection/HomeSetRefresher.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/at/bitfire/davdroid/servicedetection/HomeSetRefresher.kt
Outdated
Show resolved
Hide resolved
…etRefresher.kt Co-authored-by: Copilot <[email protected]>
sunkup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More detailed then necessary. See comments.
Otherwise its nice, clean and works really well. 👍
app/src/main/kotlin/at/bitfire/davdroid/servicedetection/HomeSetRefresher.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/at/bitfire/davdroid/servicedetection/HomeSetRefresher.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/at/bitfire/davdroid/servicedetection/HomeSetRefresher.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/at/bitfire/davdroid/servicedetection/HomeSetRefresher.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed something and since this is related to personal collections, here is another quick question about code in shouldPreselect:
When Settings.PRESELECT_COLLECTIONS_PERSONAL is selected, the code checks if the collection's homeSetId is in the list of personal home-set IDs. However, if the collection's homeSetId is null (which can happen if the collection is without a home-set, right?), this check will fail.
I guess that's what we want (if collection has no homeset it can't be personal and should not be preselected), but I think it's not clear that the code behaves this way. If I am right (?) we should add a short comment to document this.
app/src/main/kotlin/at/bitfire/davdroid/servicedetection/HomeSetRefresher.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: Arnau Mora <[email protected]>
Yes, I think that if a collection doesn't have a homeset, it won't ever be personal. Maybe @rfc2822 has something else to add, but I'd just add a comment. |
Signed-off-by: Arnau Mora <[email protected]>
sunkup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but it's not done yet right?
1. Update the state
Right now the collection personal state is only saved on creation of the collection. We need to update it in refreshHomesetsAndTheirCollections and refreshCollectionsWithoutHomeSet.
2. Start using the new state
Now that we have the new personal state saved in collection we should use it:
- use
collection.personalinstead ofhomesets.personalto filter the list of collections in the query:CollectionDao.pagePersonalByServiceAndType. - update
HomeSetRefresher.shouldPreselect()to use the personal state from collection instead of home set
3. Remove personal state from home set
And then I wonder now: I think we do not need the home set personal state anymore? If we don't need it anymore we should remove it either now or in the future.
Since this change alters functionality (less collections are shown as personal, see screenshots below) and I guess it does not really hurt to query and save the home set personal state for a bit longer, maybe we should wait for a while and see whether there are any complaints?
4. Show state in collection info screen
Optionally we could also show the personal state in the collection info now.
What do you think?
rfc2822
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also #1372 (comment) – we should clarify the requirements first


Purpose
Update the current implementation (right now no home-set is ever personal), to initialize the value based on the
DAV:ownerproperty.Short description
HomeSet.personalwill be true wheneverDAV:owneris set and equal to the service's principal url.Checklist