GNOME Bugzilla – Bug 655919
Change NoteDetails.notes to support vCard-like arbitrary parameters
Last modified: 2011-08-12 16:01:27 UTC
Proposed API changes: - public abstract Set<Note> notes { get; set; } + public abstract Set<NoteFieldDetails> notes { get; set; } /* these will be covered by Note's new parent class, AbstractFieldDetails */ - public static bool equal (Note a, Note b) + public override bool equal (AbstractFieldDetails<Note> that) - public static uint hash (Note r) + public override uint hash ()
Created attachment 193490 [details] [review] Add NoteFieldDetails Relevant patch and bonus patches for fixes along the way in branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo655919-note-field-details
Review of attachment 193490 [details] [review]: Looks fine apart from a few missing/unchanged comments. A few other minor things to look at before committing, such as property types and some hunks which don't look like they belong in this patch. ::: backends/tracker/lib/trf-persona.vala @@ +235,2 @@ /** * {@inheritDoc} I think this comment needs a @since UNRELEASED. ::: folks/individual.vala @@ +414,2 @@ /** * {@inheritDoc} I think this also needs a @since UNRELEASED. ::: folks/note-details.vala @@ +35,3 @@ * The UID of the note (if any). */ public string uid { get; set; } If we're allowing nulls to be passed through from the constructor, this property should have type “null?”. @@ +43,3 @@ + * @param parameters initial parameters. See + * {@link AbstractFieldDetails.parameters}. A `null` value is equivalent to a + * empty map of parameters. Missing documentation for @param uid. @@ +86,3 @@ + + if (this.uid != null) + retval += this.uid.hash (); I'd use XORing here instead of addition. I recall reading somewhere that it gives a more uniform distribution of hash values (whereas addition tends to favour large numbers for hash values more). @@ +104,2 @@ * * @since 0.5.1 Should be @since UNRELEASED. ::: tests/eds/Makefile.am @@ +78,3 @@ FOLKS_BACKEND_STORE_KEY_FILE_PATH=$(backend_store_key_file) \ AVATAR_FILE_PATH=$(avatar_file) \ + FOLKS_WRITEABLE_STORE="eds:local://test" \ Not sure this belongs in this patch. ::: tests/eds/link-personas.vala @@ +235,3 @@ + (GLib.HashFunc) ImFieldDetails.hash, + (GLib.EqualFunc) ImFieldDetails.equal); + im_addrs2.set ("yahoo", new ImFieldDetails (this._im_address_2)); I don't think these belong in this patch. ::: tests/eds/set-im-addresses.vala @@ +122,3 @@ + (GLib.EqualFunc) ImFieldDetails.equal); + im_addrs.set ("jabber", + new ImFieldDetails ("bernie@example.org")); Not sure this belongs in this patch.
commit b8c144844c7012fcc1c49d46a9fc28de5a100aaf Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Mon Aug 8 16:08:36 2011 +0200 Rebase NoteDetails.notes upon NoteFieldDetails Closes: bgo#655919 - Rebase NoteDetails.notes upon an AbstractFieldDetails-derived class