[Kde-pim] Patch for KMail : unification between KMail tags and Nepomuk tags

[Adrien BUSTANY]

Thomas McGuire a écrit :
> Hi,
>
> On Tuesday 29 July 2008 14:01:40 Adrien BUSTANY wrote:
>   
>> I wrote a short patch for KMail which improves the Nepomuk integration.
>>     
>
>   
>> It's all ifdef'ed, so the non-nepomuk behaviour stays the same. The
>> patchs cleans the tag manager a bit too, making it more robust (no more
>> MessageTag #).
>>     
> See below why I don't like the changes to the config entry layout.
>
>   
>> What it does is to use Nepomuk as a backend for tags, instead of the
>> KMail rc file. There are still informations about tags in the rc file,
>> like keybindings and colors, but the list of tags is stored in KMail.
>> This makes the desktop more coherent when using Nepomuk. If a tag is
>> added in KMail's config window, you'll see it in Dolphin and other
>> Nepomuk enabled apps too. Removing a tag works the same way.
>> One thing I can't explain is that I hit a QList assert when using
>> QList::contains on an empty list, hence the two stupid if in the patch.
>>     
>
>   
>> One thing I'd like to do is to call writeConfig when the user closes the
>> settings window, so that the tag changes are immediatly visible in other
>> apps. It's not in the patch as I wanted to know your point of view about
>> that.
>>     
> I'm OK with that, it is much safer anyway, it would prevent loosing the 
> updated tags when KMail crashes.
>
> Some random comments on the patch:
>
> - The most important thing is config compatibility: User should not loose 
>   their old tags, and tagged messages should still be tagged the same way.
>   You should not change the format of the config entries in any way, unless 
>   you write an update script (see the kconf_update subdirectory).
>   Please make sure everything is still compatible.
>   I'd say the easiest way is to not change the config entry layout (that would 
>   also reduce the diff and make it a bit more easier to read)
>   
Well, not changing the config is obviously the easiest way, but the 
original config layout is very strange imho. On the other hand, and 
update script would be tricky since you have to enter the existing tags 
into Nepomuk...
> - // This if is stupid, but without it I get an assert (dunno why).
>   Find the real reason for this assert instead of coding around something you 
>   don't understand. If this problem still occurs in the next patch, I can help 
>   you with that.
>   
Yep, I have to find the reason for that.
> - I am not quite sure about the // Sync the changes back to Nepomuk
>   That seems like it will destroy all tags in Nepomuk which are not in KMail's 
>   tag list, right? So it seems to me that when the user creates a tag in  
>   Dolphin while KMail is open, that tag is deleted when KMail closes, which 
>   doesn't seem good.
>   
That is a known problem, we will likely add hooks to be notified when 
the tag list changes. I did not really see another way to sync the tags.
> - Even with Nepomuk enabled, I'd still like to have the example tags. Maybe 
>   they are not needed if there are already existing Nepomuk tags, but that is 
>   not the default, right?
>   
The example tags can of course be there, there are no default tags in 
Nepomuk but that is not a problem.
> - Do not duplicate the KMMessageTagDescription constructor. In fact, you've 
>   duplicated a bug here, the icon name should be "mail-tagged", not 
>   "feed-subscribe", IIRC.
>   I suggest getting rid of most of the parameters in the constructor and using 
>   setters instead, and only use two small constructors that set the label (one 
>   automatically, one as parameter). The default values could be set in an 
>   init() function called by the two constructors. See also:
>   http://doc.trolltech.com/qq/qq13-apis.html#theconveniencetrap
>   
So we're breaking the old API here ?
> - Use QString(), not ""
>   
OK
> - Please respect the KMail coding style more, see 
>   http://websvn.kde.org/*checkout*/trunk/KDE/kdepim/kmail/HACKING?revision=839299
>   (To be fair, that file was only added a few days ago)
>   
I'll take that into account
> - // Ensure we won't have any duplicate tag
>   I think this should be done in the UI instead, i.e. if the user enters a 
>   tag name which already exists, don't enable the add tag button, so the user
>   can not enter duplicate tags.
>   
That'd be better from a usability point of view
> - Why do french people always write their last name in all capitals? No 
>   offence meant, I'm genuinely curious.
>   
Errr, I guess that's a convention. Like "why do english people always 
capitalize first letters of every word in titles ?"... I won't be 
offensed if I have to write it lowercase though :D
> Note that I didn't try the patch yet, as I didn't want to loose my tag 
> settings. I assume you've tested it, though :)
>
> Please send a new patch addressing the issues, especially the ones about 
> config compatibility.
>
> Is there a GUI for Nepomuk which allows seeing the current list of tags (and 
> whatever else Nepomuk stores in its database)?
>   
You can see the tags with Dolphin for example, but there's no generic 
resource brother yet (well, the krunner module in the playground is a 
sort of browser, but you cannot do a lot of things with it).
> Thanks for the patch, better Nepomuk integration surely is good!
>   
Actually I'm really split between "I should wait before KMail switches 
to Akonadi", which means not too soon I guess, and "it's not too hard to 
hack around, but it's still a hack". I'll try to make the patch cleaner 
though, as we really "need" to have some useful things to do with Nepomuk.

Also, thanks for the detailed review, I appreciate it :-)
> Cheers,
> Thomas
>
>   
Best regards
Adrien

>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> KDE PIM mailing list kde-pim at kde.org
> https://mail.kde.org/mailman/listinfo/kde-pim
> KDE PIM home page at http://pim.kde.org/

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/