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

[Inge Wallin]

On Monday, April 28, 2014 00:50:31 Frederik Gladhorn wrote:
> Hi, I just saw the patch and thought I'd add my 2 cents, feel free to ignore
> me.

Hi Fredrik,  I wouldn't dream of ignoring you!  :)

> 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.

And I think it still is. I only added data to the private class and methods to 
the public class - which is indeed the very purpose of using a private class 
at all. I agree about your comment about KF5. We should always strive for 
backward compatibility which I think, unfortunately, that KDE has a history of 
not fully doing.

> 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.

I agree, the names are not ideal.  Before comitting, I spent a good deal of 
time trying to find better ones but I simply couldn't come up with anything. As 
you probably know by now, I am not very fond of the name "grade" for something 
which is not a grade at all - rather it's a level or degree of confidence as 
somebody wrote on the forum. But I can't change anything in the old library 
without destroying backward compatibility completely.

"interval" is the one that I think makes the most sense. It is exactly what it 
says, namely the interval until the next time when the application thinks the 
word should come up again. It is correct that the time when it was last 
practiced is stored in the file, and without this information the interval is 
meaningless. 

"pregrade" is indeed bad, and I would love to come up with something better. 
But in my opinion, it's bad because "grade" itself is bad. The comment could 
also indeed be improved, and I will do that. :)  As you probably understood 
from my review request, although not obvious from the code, is that the 
intention is to use it exactly like "grade", but for the very early phases in 
the training when the word needs to be shown much more often than once a day.

And I know that "interval" and "pregrade" are redundant. Pregrade is a 
discreet system that puts the responsibility of finding out when to show the 
word again on the application. Interval stores the same information in the file 
and is continuous, i.e. you can store any value there with one second 
resolution. For practical purposes one of them is enough.

But I wanted both there so that I could experiment with the approaches during 
the 4.14 development cycle. When the time to release comes we can either just 
let it be to allow both approaches in the future (I'm quite fond of ditching 
the grades altogether and just use interval, myself, but others didn't like 
that). Or we can select one and just remove the other. They are pretty 
independent anyway.

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

Please help out. So far I have failed, at least with pregrade. :)

> 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 ;)

It's more than a good idea, it's necessary in my opinion.

Thanks for your input!

	-Inge

> 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/8c03571a/attachment.html>