[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [coldsync-hackers] record attributes madness
On Sat, Oct 11, 2003 at 10:22:59AM -0400, Christophe Beauregard wrote:
> I've been trying to get my head around the state of attribute naming in
> various parts of ColdSync. It's not pretty.
>
> Just to establish a baseline, PalmOS SDK 5 DLCommon.h:
> // Database record attributes
> #define dlpRecAttrDeleted 0x80 // delete this record next sync
> #define dlpRecAttrDirty 0x40 // archive this record next sync
> #define dlpRecAttrBusy 0x20 // record currently in use
> #define dlpRecAttrSecret 0x10 // "secret" record
> #define dlpRecAttrArchived 0x08 // archived record
>
> DataMgr.h uses Delete rather than Deleted and doesn't have archive, but
> otherwise it's about the same. I _do_ wonder what the Palm coder that wrote
> those comments was smoking, though.
>
> ColdSync's pdb.h:
> #define PDB_REC_EXPUNGED 0x80
> #define PDB_REC_DIRTY 0x40
> #define PDB_REC_DELETED 0x20
> #define PDB_REC_PRIVATE 0x10
> #define PDB_REC_ARCHIVE 0x08
If I may give my two cents' worth: as might be obvious from
the comments in "include/pdb.h", I wasn't terribly happy with Palm's
identifiers. That's how "deleted" mutated into "expunged", and "busy"
into "deleted".
The version in "pdb.h" is the oldest piece of code of all the
ones you cite, and the one that I'd favor as the standard.
> Palm::PDB:
> $entry->{attributes}{expunged} = 1 if $attributes & 0x80;
> $entry->{attributes}{dirty} = 1 if $attributes & 0x40;
> $entry->{attributes}{deleted} = 1 if $attributes & 0x20;
> $entry->{attributes}{private} = 1 if $attributes & 0x10;
> $entry->{'attributes'}{'Delete'} = 1 if $attributes & 0x80;
> $entry->{'attributes'}{'Dirty'} = 1 if $attributes & 0x40;
> $entry->{'attributes'}{'Busy'} = 1 if $attributes & 0x20;
> $entry->{'attributes'}{'Secret'} = 1 if $attributes & 0x10;
> and $entry->{attributes}{archive} for 0x08
>
> ColdSync::SPC:
> $retval->{'attributes'}{'deleted'} = $retval->{'attrs'} & 0x80 ? 1 : 0;
> $retval->{'attributes'}{'dirty'} = $retval->{'attrs'} & 0x40 ? 1 : 0;
> $retval->{'attributes'}{'busy'} = $retval->{'attrs'} & 0x20 ? 1 : 0;
> $retval->{'attributes'}{'secret'} = $retval->{'attrs'} & 0x10 ? 1 : 0;
> $retval->{'attributes'}{'archived'} = $retval->{'attrs'} & 0x08 ? 1 : 0;
>
> I'm sure you see where I'm going with this.
>
> The short summary is that any chunk of code which mixes ColdSync::SPC
> records with Palm::PDB records is in trouble.
Gah! You're right.
Alessandro, in case I hadn't pointed it out before: this looks
as if it might introduce user-visible changes, and break existing
code. In the past, when this has happened, I've incremented the minor
version number (e.g., 1.1 -> 1.2) even if it was a comparatively small
change.
As a side issue, it looks as though the capitalized attributes
in "Palm/PDB.pm" use the "official" identifiers from the SDK. This
looks a tad dangerous to me, since it can introduce subtle bugs: if
you create a record, and (accidentally?) have
$record->{attributes} = {}
$record->{attributes}{Busy} = 1;
$record->{attributes}{deleted} = 0;
what will happen? If "Busy" and "deleted" are synonyms, then after
this code fragment, one would like
$record->{attributes}{Busy} == 0 and
$record->{attributes}{deleted} == 0;
but that's not what happens.
One solution would be to have $record->{attributes} be a tied
hash that performs magic behind the scenes to make the Right Thing
happen. But that seems like overkill.
Another solution would be to recommend the use of OO accessors
to set attributes:
$record->set_attribute('Busy');
$record->unset_attribute('deleted');
or some such. But this violates the KISS principle.
I think the best solution might be to find those sets of
identifiers that refer to the same flags ("Busy" and "deleted",
"Secret" and "private", etc.), and issue a warning in Palm::PDB::Write
if they are used. (I'd love to check for those at compile-time, but
can't think of any way to do so.) Perhaps add a flag so that
use Palm::PDB("sdk_attrib_id_compat");
doesn't issue warnings.
> ColdSync::PDB is an obvious
> candidate,
Candidate for what? I'm not quite following you.
> but I was just in the process of migrating an algorithm from
> Palm::PDB to ColdSync::SPC and realized that there's no expunged attribute
> and "deleted" maybe doesn't mean "deleted". The "archive" to "archived"
> change seems minor in comparison.
>
> As well, the pdb.h/Palm::PDB 'deleted' attribute just seems wrong. I assume
> there's a historical reason for this? The 'busy' (0x20) flag was just never
> set on sync?
When I first started playing with this stuff, I found that the
identifiers in Palm's SDK didn't seem to correspond to reality, at
least not the way I saw it.
What they called "busy" didn't mean busy at all, it meant that
the user had deleted the record. To me, "busy" implies that the record
is temporarily inaccessible, and that's not what's going on at all. So
I renamed 0x20 to "DELETED".
This meant that I had to rename 0x80 to something else. Since
this flag means that the body of the record has been erased on the
PDA, I needed something stronger than "deleted", and called it
"expunged".
The change from "archive" to "archived" is a change from an
adjective to an imperative verb: at sync time, the record has not yet
been archived. Rather, this is a command to HotSync to archive the
record.
> My gut instinct at this point is to add the PalmOS 5 naming conventions
> (Delete, Dirty, Busy, Secret, Archive) to ColdSync::SPC and just stick with
> those for any future work.
I disagree: I think the identifiers in "include/pdb.h" are the
most descriptive and intuitive ones; everything else should be based
on those.
> Changing around the semantics of Palm::PDB would
> be a Bad Thing since it seems to have a pretty large installed base.
> ColdSync::SPC could change, of course, but it seems to be the only part of
> ColdSync that matches the SDK-5 attribute naming conventions :(
>
> Comments?
I feel that readability is more important than adherence to
the SDK. It's not as if anyone is expected to use Palm's SDK directly
when writing ColdSync or Palm::PDB code.
But thanks for pointing this out. I hope my comments made sense.
--
Andrew Arensburger This message *does* represent the
arensb@ooblick.com views of ooblick.com
Once we've got the bugs ironed out, we'll be running on flat bugs.
This message was sent through the coldsync-hackers mailing list. To remove
yourself from this mailing list, send a message to majordomo@thedotin.net
with the words "unsubscribe coldsync-hackers" in the message body. For more
information on Coldsync, send mail to coldsync-hackers-owner@thedotin.net.