Review Request 117730: keduvocdocument: implement data fields necessary for initial learning in Parley

[Frederik Gladhorn]

Hi, I just saw the patch and thought I'd add my 2 cents, feel free to ignore me.

I'd like to note that when you make changes to the library,
please be aware that this can potentially effect other apps. So far the kvtml lib has 
stayed binary and source compatible over the course of KDE 4.

Maybe that is silly, in which case you can change it at any time since it won't be 
released independently of the kde edu apps. Stating in the docs if compatibility is 
kept or not may be a good idea, I always thought it has to stay compatible. When 
moving to KF5 at some point is a good idea to make the old xml "read-only" in case 
you plan to make bigger changes.

I would re-consider the naming of the api you added though. I don't understand what 
it's good for by reading just the function names/xml tags.

"interval" as far as I understood is some sort of time indicator when the word should 
come up again, but as Jeremy noted, we have the date when it was last shown.

"preGrade" just doesn't make sense for it, adding a comment that the pregrade is 
the pregrade as documentation doesn't really help explaining it either.

Since this is a library, I'd recommend spending a bit more time to come up with 
descriptive function names :)

Being able to load old file is really a good idea (in that sense the patch is good) and 
people will come after you if you break their carefully crafted collection of 20000 
words ;)

Greetings,
Frederik

On Thursday 24. April 2014 03.35.15 Inge Wallin wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117730/
> -----------------------------------------------------------
> 
> Review request for KDE Edu and Amarvir Singh.
> 
> 
> Repository: libkdeedu
> 
> 
> Description
> -------
> 
> This patch implements two new fields in a kvtml file:
>  - pregrade, which is equal to grade but used in the interval 0-1 days as
> opposed to grade which is used for intervals 1 day and up. - interval,
> which is used to indicate how long since a word was last trained it is due
> for training again.
> 
> This is prerequisite to the implementation in Parley of the initial
> training. The reason for implementing these two fields instead of changing
> old ones is that it will still be backward compatible. I think this is
> important, especially in the light of the ktouch issue that we saw on the
> mailing list a few days ago.
> 
> 
> Diffs
> -----
> 
>   keduvocdocument/keduvoctext.h 1d196c2
>   keduvocdocument/keduvoctext.cpp 41d44d0
>   keduvocdocument/kvtml2defs.h d2903ed
> 
> Diff: https://git.reviewboard.kde.org/r/117730/diff/
> 
> 
> Testing
> -------
> 
> Not much testing is done, that will happen when I implement it in parley.
> This review is mostly to get an ok on the new data fields in general. The
> testing that I have done so far is to guarantee that keduvocdocument still
> works as before for code not using the new API.
> 
> 
> Thanks,
> 
> Inge Wallin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20140428/16d7207b/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20140428/16d7207b/attachment-0001.sig>