> On July 29, 2013, 4:04 p.m., Vishesh Handa wrote: > > The main thing that I dislike about this patch is the fact that it adds > > methods that are very specific to a certain use case in a public API. Do > > you think you could just make it work with a setUserData(void* ) or > > something similar? That way anyone who needs to store additional info in a > > Term can use that. > > Denis Steckelmacher wrote: > It is a good idea, but how do we handle the case of several bits of code > that each want to call setUserData ? For instance, a library can get a > Nepomuk2::Query parsed using the new query parser (that has called > setUserData for every term), and it may also want to store something. This > problem can be solved by using setUserData(const QString &key, void *), but > this would be slow and heavy. > > There is also the case of code that want to access the positional > information. If the code is in Nepomuk, it will have to include a private > header describing the user data format used by the parser (and this format > will have to be binary-compatible between versions if user can update > nepomuk-widgets without updating nepomuk-core, or vice versa). > > Another possible solution is to keep the positional information in > TermPrivate, and to put a "friend class Nepomuk2::QueryBuilder" at the top of > Term. This way, there is nothing exposed in the public API, and QueryBuilder > can access the positional information through the d-pointer of Term.
Further reflection made me think about two possibles ways to enhance this patch : * Add three methods : bool setUserData(TermUserData *), void clearUserData() and TermUserData *userData(). The goal is to prevent an application to overwrite the user data of a library by putting checks against that : setUserData will only succeed if there were previously no user data associated with the term. clearUserData removes the user data of the term (and deletes it, it is the reason why a TermUserData class having a virtual destructor is used). userData simply returns the user data currently associated with the term. * Add two methods : void setUserData(const QString &key, void *) and void *userData(const QString &key). Here, a string key is used to disambiguate the user data of different users of the Term class. This would require to add a QHash<QString, void *> in TermPrivate. Personally, I prefer the first solution, as it is more lightweight that the second one (TermPrivate still contains only a pointer). I think it can be used because it will be very rare that more than one user of the Term class will need to store user data in it. If it is the case, then one of the users can find a way to be compatible with the other (for instance, user A can replace its user data with the one of user B just before calling a user B method on the term). What do you think ? - Denis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111274/#review36754 ----------------------------------------------------------- On June 27, 2013, 6:45 p.m., Denis Steckelmacher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111274/ > ----------------------------------------------------------- > > (Updated June 27, 2013, 6:45 p.m.) > > > Review request for Nepomuk. > > > Description > ------- > > This patch adds two fields to Nepomuk2::Query::Term: "position" and "length". > By default, they are zero and everything works as before. This change is > binary- and source-compatible. > > The in-development Nepomuk query parser (GSoC 2013 project) uses these fields > to map parsed terms to the original user query string. By traversing the > parse tree (a tree of And, Or and Comparison terms), it is therefore possible > to match every term with a piece of input, that can be highlighted > accordingly. For instance, each ComparisonTerm can have a different color, > its subTerm being rendered using a bold font (like in "sent to *Jimmy*"). > > This patch also makes the QuerySerializer serialize the positional > information, for debug purposes. This information is not read-back by the > deserializer, as the modification would be fairly intrusive, and there is no > need to deserialize positional information if the original user query is lost. > > > Diffs > ----- > > libnepomukcore/query/queryserializer.cpp 7ac6131 > libnepomukcore/query/term.h 114baa3 > libnepomukcore/query/term.cpp cfb41f4 > libnepomukcore/query/term_p.h 6d92b48 > > Diff: http://git.reviewboard.kde.org/r/111274/diff/ > > > Testing > ------- > > The query parser (https://github.com/steckdenis/nepomukqueryparser) is able > to store positional information into terms, and this information is valid. > The query "files related to mails sent by John, dated of last week" produces > the following term tree: > > <and position="0" length="55"> > <type position="0" length="5" uri="FileDataObject"/> > <comparison position="6" length="30" property="relatedTo" comparator="=" > inverted="false"> > <and position="0" length="35"> > <type position="17" length="5" uri="Message"/> > <comparison position="23" length="12" property="messageFrom" > comparator=":" inverted="false"> > <literal position="31" length="4" datatype="string"> > John > </literal> > </comparison> > </and> > </comparison> > <and position="46" length="9"> > <comparison position="46" length="9" property="fileLastModified" > comparator=">=" inverted="false"> > <literal position="46" length="9" datatype="dateTime"> > 2013-06-16T22:00:00.002Z > </literal> > </comparison> > <comparison position="46" length="9" property="fileLastModified" > comparator="<=" inverted="false"> > <literal position="46" length="9" datatype="dateTime"> > 2013-06-23T20:00:00.002Z > </literal> > </comparison> > </and> > </and> > > > Thanks, > > Denis Steckelmacher > >
_______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
