[Bug 27271] API review: TplLogEntry, TplLogEntryText

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27271

Cosimo Alfarano <cosimo.alfarano at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://git.collabora.co.uk/
                   |                            |?p=user/kalfa/telepathy-log
                   |                            |ger.git;a=shortlog;h=refs/h
                   |                            |eads/log-entry-api-review

--- Comment #6 from Cosimo Alfarano <cosimo.alfarano at collabora.co.uk> 2010-04-28 10:14:18 PDT ---
This branch should still wait for some other branch to be merged.


> Will code outside the library need to subclass TplLogEntry? If not, 
> > we could move TplLogEntryClass to a log-entry-internal.h, reducing our
> > API surface area.
> 
> No, subclassing is just for who wants to extend the logger, so only within the
> logger until TPL will support plug-ins
> 
> I'll move the class to -private.

Moved.

> > #define TPL_LOG_ENTRY_MSG_ID_IS_VALID(msg) (msg >= 0)
> > #define TPL_LOG_ENTRY_MSG_ID_UNKNOWN -2
> > #define TPL_LOG_ENTRY_MSG_ID_ACKNOWLEDGED -1
> 
> > I don't like this overloading of message IDs. I think the log entry should 
> > have a separate flag for whether the message has been acknowledged, 
> > and perhaps also a flag for whether the message ID is meaningful.
> 
> the pending_msg_id bits are used only to be able to understand what, in the
> pending message list, has been already logged, at TPL start up.
> 
> Yeah, it might confuse people when using such values in TPL and with different
> semantic in tp-glib.
> 
> I'll add another method with a flag.

Added _pending_status() methods instead.

Now before getting the pending message id a caller should always check if the
pending status for the log entry is set to PENDING, otherwise the msg_id is
meaningless.

This is the reason for which I extended the msg_id possible values to negative,
even if originally they were unsigned, so I could exclusively set the msg_id
(zero-postive) or the alternative meta-value for it (negative).

I'd like there were a possible value for msg_ids that means not-set.

tpl_log_entry_text_set_pending_msg_id() returns 0 on errors, which is actually
OK, since it can happen only on wrong instance of @self with g_return_*

But I'd like to avoid that someone use _get_pending_msg_id() considering its
value as a valid id, when actually the _get_pending_status() is not PENDING.

It seems that there is nothing I can do about it, but document it.

In some cases I needed to set a improbable value for the msg-id, for example in
log-store-xml.c:829
channel-text.c:763 and 1179
log-entry-text.c:263 and 365
and some other places.

I don't feel really satisfied with it, though.


> > gint tpl_log_entry_get_pending_msg_id (TplLogEntry *self);
> >> Will API users ever care about this? It'd be good if this could be internal
> >> to the logger and the library.
> 
> OK.

Actually Empathy uses it. I think that it makes a wrong use of pending messages
and it can be replaced, I'll need to check Empathy for it and ask the E. team.

In the meantime I flag them as possible-internal methods.


> > gboolean tpl_log_entry_is_pending (TplLogEntry *self);
> >> This only seems to make sense for Text messages? Perhaps it should 
> >> only exist in the subclass?
> 
> ok.

superseded by tpl_log_entry_text_get_pending_status(), as above.

all the methods about pending msg ids are not in TplLogEntryText, since for
what I understand there the concept of "pending/ack'd" is not a generic concept
but just some channels have it.

> > const gchar *tpl_log_entry_get_channel_path (TplLogEntry *self);
> >> What's this for? Does anything use it? Channel paths aren't unique 
> >>or meaningful over time, so it seems unwise.

> I understand your point but I needed it. I think you suggested a similar
> approach in the past, but if you have a way to remove it, I'd be glad to get
> rid of lots of methods carrying transient of misleading data.
> 
> This method would only be internal.

I moved _{s,g}et_channel_path and _get_account_path to -internal.h

This way no public misuse of paths can be done.

If you have a better way to handle the situation, I'd be glad to adopt it.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.