dee

Merge lp://qastaging/~mhr3/dee/more-props into lp://qastaging/dee

Proposed by Michal Hruby
Status: Merged
Approved by: Mikkel Kamstrup Erlandsen
Approved revision: 364
Merged at revision: 353
Proposed branch: lp://qastaging/~mhr3/dee/more-props
Merge into: lp://qastaging/dee
Prerequisite: lp://qastaging/~mhr3/dee/more-tests
Diff against target: 890 lines (+512/-30)
10 files modified
src/dee-peer.c (+67/-4)
src/dee-peer.h (+10/-8)
src/dee-shared-model.c (+92/-11)
src/dee-shared-model.h (+28/-3)
tests/Makefile.am (+11/-3)
tests/model-helper-append1.c (+55/-0)
tests/model-helper-replace.c (+70/-0)
tests/test-client-server.c (+63/-1)
tests/test-model-interactions.c (+106/-0)
vapi/dee-1.0.vapi (+10/-0)
To merge this branch: bzr merge lp://qastaging/~mhr3/dee/more-props
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve
Review via email: mp+94746@code.qastaging.launchpad.net

Description of the change

Added extra properties to help us deal with the problem of remote peers writing to a model which they shouldn't.

Added tests for the new properties. UNBLOCK

To post a comment you must log in.
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Why rename *self to *peer in all method args? That goes the conventions used elsewhere in Dee..?

272 + * Note that this does NOT mean that the peer is owner of the swarm! Check also
273 + * dee_peer_is_swarm_leader().

s/peer is owner/peer is leader/

375 /**
376 + * DeeSharedModel:disable-remote-writes:
377 + *
378 + * Boolean property defining whether or not the model will accept changes
379 + * done by remote peers.
380 + *
381 + * This is useful when one process is considered an "owner" of a model and
382 + * all the other peers are supposed to only synchronize it for reading.
383 + * Note that this will only have effect if the associated peer is swarm
384 + * leader.
385 + */

Can you add a cross ref to the DeePeer:swarm-owner property

On "disable-remote-writes": I think we need to turn this into a set of flags or an enum in order to ensure forward compat. And enum is probably best. This way we can prevent us getting into an n! combinatorial explosion (or worse, semantic ambiguity) if we need to add new access modes.

I am thinking; call the property "access-mode" and define a DeeSharedModelAccessMode enumeration with two values (for now) DEE_SHARED_MODEL_ACCESS_MODE_WORLD_WRITABLE (the default value) and DEE_SHARED_MODEL_ACCESS_MODE_LEADER_WRITABLE.

We might also want to add client side validation on the writability of the model... (Just logging a critical and leaving the model unchanged) In order to catch programming errors more easily and not leading to desktop crunch if we get into a Invalidate/Clone loop... I think this could be done cleanly in DeeProxyModel with a "writable" boolean property (which we can then bind to "is leader" to state of the DeeSharedModel)

And finally - does the testing also apply against the p2p mode of DeeSharedModel? I think that needs to be validated...

review: Needs Fixing
Revision history for this message
Michal Hruby (mhr3) wrote :

Ok, most of these should be fixed, (minus the writable prop on ProxyModel), I'm still unsure about constructors taking the access_mode - do we want to leave this up to apps to call g_object_new with all the props themselves, or are we going for a new constructor (I really don't like that idea if it's supposed to do the initial Clone as well).

We already have quite a few of the constructor variants, do we really want dee_shared_model_new_for_peer_with_backend_accessed_as(Peer, Model, AccessMode)?
Drop the peer? - then there's no simple way to use it with DeeClient/Server
Drop the model? - that probably makes sense if we're doing initial Clone anyway, then again DeeServer can't do initial Clone

Or perhaps just do a static method that will try to do the initial clone, and leave the constructors alone? Sounds most sane to me.

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

I think the role of the remote-writes-disabled property is unclear. It is implied by the value of access-mode property in almost all cases, and checking for weird combinations of these properties might make the API more confusing than good is.

Also; if we do want to keep disable-remote-writesthen I think we should rename it to a non-verb form ala remote-writes-disabled.

96 + * DeePeer::swarm-owner:

Use only one single colon here, double colons are for signals. As it is the docs doesn't pick up this comment.

147 +dee_peer_get_swarm_owner (DeePeer *self)

Should be *_is_swarm_owner() to be consistent with the other boolean properties. Same applies for the disable_remote_writes if we keep it.

281 + * This is useful when a model created with ::access-mode set to

Property refs in gtk-doc needs single colons. The refs to the DeeSharedModelAccessMode enum values in this chunk also needs #-prefixes to give correct cross refs.

302 + * Setting this to DEE_SHARED_MODEL_ACCESS_MODE_LEADER_WRITABLE is useful

Should be #DEE_SHARED_MODEL_ACCESS_MODE_LEADER_WRITABLE (hash prefix, again)

306 + * See also DeePeer::swarm-owner property to ensure ownership of a swarm.

Double colon -> single colon

465 +typedef enum
466 +{
467 + DEE_SHARED_MODEL_ACCESS_MODE_WORLD_WRITABLE,
468 + DEE_SHARED_MODEL_ACCESS_MODE_LEADER_WRITABLE
469 +} DeeSharedModelAccessMode;

Should be documented.

418 + * the ::disable-remote-writes property to ignore any misbehaving clients
438 + * Gets value of the ::disable-remote-writes property.

Single colon for property xref.

review: Needs Fixing
Revision history for this message
Michal Hruby (mhr3) wrote :

As discussed on IRC, the disable-remote-writes property was removed.

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Ok, I am happy :-) Great work!

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

The prerequisite lp:~mhr3/dee/more-tests has not yet been merged into lp:dee.

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: