After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 652657 - Allow writing to properties of all personas
Allow writing to properties of all personas
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: libfolks
git master
Other All
: Normal normal
: folks-0.6.0
Assigned To: folks-maint
folks-maint
Depends on: 650422
Blocks: 655911
 
 
Reported: 2011-06-15 15:13 UTC by Philip Withnall
Modified: 2011-08-03 21:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Clarify the documentation of PersonaStore.is_writeable (1.43 KB, patch)
2011-08-02 22:22 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2011-06-15 15:13:46 UTC
Allow writing back to properties of personas on persona stores which are not the main persona store (i.e. with is_writeable = false). This will allow for personas to be edited individually by gnome-contacts, while still preserving the semantics that editing properties of an individual will only edit them on the personas in that individual which come from the writeable backend.
Comment 1 Travis Reitter 2011-06-16 14:37:46 UTC
(In reply to comment #0)
> Allow writing back to properties of personas on persona stores which are not
> the main persona store (i.e. with is_writeable = false). This will allow for
> personas to be edited individually by gnome-contacts, while still preserving
> the semantics that editing properties of an individual will only edit them on
> the personas in that individual which come from the writeable backend.

Don't you mean (is_writeable = true, is_primary = false)?
Comment 2 Philip Withnall 2011-06-16 15:01:18 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > Allow writing back to properties of personas on persona stores which are not
> > the main persona store (i.e. with is_writeable = false). This will allow for
> > personas to be edited individually by gnome-contacts, while still preserving
> > the semantics that editing properties of an individual will only edit them on
> > the personas in that individual which come from the writeable backend.
> 
> Don't you mean (is_writeable = true, is_primary = false)?

Nope, I mean (is_writeable = false). Currently (using alias as an example):
 • Setting Individual.alias propagates the update to the personas in the individual whose stores have is_writeable = true. This doesn't respect IndividualAggregator.primary_store.
 • Setting Persona.alias propagates the update to Telepathy. This doesn't respect is_writeable or primary_store.
 • However, IndividualAggregator ensures that only one PersonaStore has (is_writeable = true), and that is the PersonaStore which is the primary_store.

I think what we want is to limit Persona.alias updates to personas whose PersonaStores have (is_writeable = true). Other than that, our current behaviour is OK, I think.
Comment 3 Travis Reitter 2011-06-20 22:59:00 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > Allow writing back to properties of personas on persona stores which are not
> > > the main persona store (i.e. with is_writeable = false). This will allow for
> > > personas to be edited individually by gnome-contacts, while still preserving
> > > the semantics that editing properties of an individual will only edit them on
> > > the personas in that individual which come from the writeable backend.
> > 
> > Don't you mean (is_writeable = true, is_primary = false)?
> 
> Nope, I mean (is_writeable = false). Currently (using alias as an example):
>  • Setting Individual.alias propagates the update to the personas in the
> individual whose stores have is_writeable = true. This doesn't respect
> IndividualAggregator.primary_store.
>  • Setting Persona.alias propagates the update to Telepathy. This doesn't
> respect is_writeable or primary_store.
>  • However, IndividualAggregator ensures that only one PersonaStore has
> (is_writeable = true), and that is the PersonaStore which is the primary_store.
> 
> I think what we want is to limit Persona.alias updates to personas whose
> PersonaStores have (is_writeable = true). Other than that, our current
> behaviour is OK, I think.

Right. This sounds OK apart from the slight confusion that clients can write to Personas whose store has "is_writeable = false".

It'd be better if it made the behavior you describe a little more obvious, but it doesn't seem worth an API break. Maybe we could add "individual-change-writebacks" or similar and tie it to the value of "is-writeable" (with clear notes in the docs that "is-writeable" is deprecated and to use the newer property.
Comment 4 Philip Withnall 2011-06-22 10:53:10 UTC
(In reply to comment #3)
> It'd be better if it made the behavior you describe a little more obvious, but
> it doesn't seem worth an API break. Maybe we could add
> "individual-change-writebacks" or similar and tie it to the value of
> "is-writeable" (with clear notes in the docs that "is-writeable" is deprecated
> and to use the newer property.

I'm not quite sure what you mean here.
Comment 5 Travis Reitter 2011-06-22 15:06:47 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > It'd be better if it made the behavior you describe a little more obvious, but
> > it doesn't seem worth an API break. Maybe we could add
> > "individual-change-writebacks" or similar and tie it to the value of
> > "is-writeable" (with clear notes in the docs that "is-writeable" is deprecated
> > and to use the newer property.
> 
> I'm not quite sure what you mean here.

The idea being that it'd probably be easier to understand if the property suggested that writes to the individual will be pushed out to this PersonaStore, not whether the PersonaStore is capable of being written to at all.

A PersonaStore with is-writable=false sounds like it wouldn't support writing in any capacity, but that's not true.
Comment 6 Philip Withnall 2011-06-23 08:52:54 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > It'd be better if it made the behavior you describe a little more obvious, but
> > > it doesn't seem worth an API break. Maybe we could add
> > > "individual-change-writebacks" or similar and tie it to the value of
> > > "is-writeable" (with clear notes in the docs that "is-writeable" is deprecated
> > > and to use the newer property.
> > 
> > I'm not quite sure what you mean here.
> 
> The idea being that it'd probably be easier to understand if the property
> suggested that writes to the individual will be pushed out to this
> PersonaStore, not whether the PersonaStore is capable of being written to at
> all.
> 
> A PersonaStore with is-writable=false sounds like it wouldn't support writing
> in any capacity, but that's not true.

Ah, right, I see what you meant by the property name now. I agree that renaming the property would make things clearer, but I'd be happy to stick with the current property name and just make sure that it's documented well. I'm not sure such a minor rename warrants the fuss of deprecating things. Your call, though.
Comment 7 Travis Reitter 2011-06-23 15:42:33 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > It'd be better if it made the behavior you describe a little more obvious, but
> > > > it doesn't seem worth an API break. Maybe we could add
> > > > "individual-change-writebacks" or similar and tie it to the value of
> > > > "is-writeable" (with clear notes in the docs that "is-writeable" is deprecated
> > > > and to use the newer property.
> > > 
> > > I'm not quite sure what you mean here.
> > 
> > The idea being that it'd probably be easier to understand if the property
> > suggested that writes to the individual will be pushed out to this
> > PersonaStore, not whether the PersonaStore is capable of being written to at
> > all.
> > 
> > A PersonaStore with is-writable=false sounds like it wouldn't support writing
> > in any capacity, but that's not true.
> 
> Ah, right, I see what you meant by the property name now. I agree that renaming
> the property would make things clearer, but I'd be happy to stick with the
> current property name and just make sure that it's documented well. I'm not
> sure such a minor rename warrants the fuss of deprecating things. Your call,
> though.

Let's just go the well-documented route.
Comment 8 Philip Withnall 2011-08-02 22:22:18 UTC
Created attachment 193109 [details] [review]
Clarify the documentation of PersonaStore.is_writeable

https://www.gitorious.org/folks/folks/commits/652657-persona-store-writeable-docs
Comment 9 Philip Withnall 2011-08-02 22:22:37 UTC
Oh bother, I forgot a NEWS entry. I'll add one before committing.
Comment 10 Travis Reitter 2011-08-03 16:56:23 UTC
Review of attachment 193109 [details] [review]:

Update the NEWS and please commit.
Comment 11 Philip Withnall 2011-08-03 21:39:42 UTC
Comment on attachment 193109 [details] [review]
Clarify the documentation of PersonaStore.is_writeable

commit 4e60d645efa3df5f3f0abd3301aeff0d369227b8
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Tue Aug 2 23:20:07 2011 +0100

    Clarify the documentation of PersonaStore.is_writeable
    
    Closes: bgo#652657

 NEWS                     |    1 +
 folks/persona-store.vala |   10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)