Review Request: User defined variables (2)

Mon Nov 28 13:48:13 GMT 2011

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > Looks quite good. I have not yet tested it. It would be good if Mek could have a look at the calligra tables changes.

I removed the Calligra Tables integration again. There is no useful way to add/remove/edit/display them atm anyways and it takes to much time for to less effect.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > libs/kopageapp/KoPADocument.cpp, lines 223-225
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42276#file42276line223>
> >
> >     Should be moved to saveOdfProlog

Done. See comment above.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > libs/kotext/KoVariableManager.cpp, lines 89-93
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42280#file42280line89>
> >
> >     Please use 
> >     
> >     d->userTypes.find(key) and check the iterator as that removes one lookup in the hash.

Done.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > libs/kotext/KoVariableManager.cpp, lines 104-105
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42280#file42280line104>
> >
> >     how about using remove to make sure it is no longer in?

It is expected (and even required) that the names are either zero or one time in the lists and not more cause else our qhash's would probably return the wrong values. The code in KoVariableManager::setValue makes sure that is so. I added two more asserts to make even more clear what is expected here.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > plugins/variables/UserVariable.cpp, line 291
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42289#file42289line291>
> >
> >     should this really be commented out?

Yes. UserVariable::propertyChanged seems to behave weird in a way that results in us earning empty value calls for whatever reason. I remember it was working once. Anyways, this commit is not about fixing all the bugs in KoVariable & friends and that call is not needed anyways.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > plugins/variables/UserVariable.cpp, line 284
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42289#file42289line284>
> >
> >     should that really be commented out?

Yes. See below.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > plugins/variables/UserVariable.h, lines 65-67
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42288#file42288line65>
> >
> >     Is it possible to move the option widget into its own class. That makes it easier for applications not based on QWidget to use it?

Sure. But I would see that as new requirement which can be done after landing. Atm it's done the same way all our other variables including our API is done what means QWidget's are a hard requirement and logic is mixed with UI. What we would do ideally is to move *ALL* the UI things into plugins that are separated from the variable-plugin (kcm-like) and then depending on the UI either the QWidget-plugins are loaded or not. But that is out-of-scope of the work done in this branch and I would prefer not to raise the requirement to finally land that branch to master without really good reasons. The branch already produces additional work-time on my end that is not required when landed into master what should be done asap imho before a few more months are needed for the next iteration.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > libs/kotext/KoVariableManager.cpp, lines 144-145
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42280#file42280line144>
> >
> >     name can type can be const

welllll... I think that is really a waste of time. I mean I am sure I can point you at 1001 places where temp-vars could be const but do we really name that in review-requests? Feels a bit like producing additional work-load with very less positive effect. I bet I could have saved 20 places where we are using such temp-vars that are not marked const yet while writing this sentence.

So, changed but please let us not spend time on such things next time.

p.s. "type" cannot be const cause it's changed a few lines below in the else-condition and now we have
const QString name = e.attributeNS(KoXmlNS::text, "name");
QString type = e.attributeNS(KoXmlNS::office, "value-type");
what I find harder to read then the first variant. But anyways...

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > libs/kotext/KoVariableManager.cpp, line 137
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42280#file42280line137>
> >
> >     This should not use namedItemNS as this will iterate over all elements which can be very long for big documents. It should use a for loop and quit it when it found the first element that should be after user-field-decls. Doing that way will not cause a slowdown in loading documents.

Will change later in the next iteration.

> On Nov. 28, 2011, 6:53 a.m., Thorsten Zachmann wrote:
> > libs/kopageapp/KoPADocument.cpp, lines 158-160
> > <http://git.reviewboard.kde.org/r/103276/diff/1/?file=42276#file42276line158>
> >
> >     This should be done in loadOdfProlog

Done. It was not done in the first place cause this is rather error-prune cause now KPrDocument and all clases that inherit from KoPADocument and reimplement the loadOdfProlog/saveOdfProlog methods NEED to call the base implementation now. In the case of Calligra Stage (aka KPrDocument) it was already missing. I fixed that too.

- Sebastian

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103276/#review8555
-----------------------------------------------------------

On Nov. 27, 2011, 3:54 p.m., Sebastian Sauer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103276/
> -----------------------------------------------------------
> 
> (Updated Nov. 27, 2011, 3:54 p.m.)
> 
> 
> Review request for Calligra and Thorsten Zachmann.
> 
> 
> Description
> -------
> 
> Following patch implements user defined variables. This solves bug https://bugs.kde.org/show_bug.cgi?id=282972
> 
> What I did;
> * extended the KoVariableManager to handle now also such user defined variables.
> * the KoVariableManager now has loadOdf and saveOdf methods to load and save user defined variables declarations.
> * the user defined variables are implemented using the new plugins/variables/User* classes.
> * KoVariable::manager() can now be used even on KoVariable::createOptionsWidget
> * replaced the previous unused KoInlineObject::User with KoInlineObject::UserGet and KoInlineObject::UserInput and make use of them
> * extended KoTextLoader.cpp to proper load user defined variables into the KoVariableManager. Instances are created using the new UserVariable plugin.
> * extended KoOdfNumberStyles with the formatFraction method. Ideally I would also move the other format (e.g. formatDate, formatTime, etc.) methods from the plugin to the KoOdfNumberStyles class to have it reusable (we at least need formatDate and KoOdfNumberStyles also in the DateVariable later).
> * added the KoOdfNumberStyles::saveOdfBooleanStyle to also save boolean formattings proper back.
> * introduced the KoOdfNumberStyles::saveOdfNumberStyle method to handle choosing the proper KoOdfNumberStyles::saveOdf*Style methods.
> * extended KWOdfWriter.cpp to proper save the user defined variable declarations back to the ODT.
> 
> Remaining problems;
> * there is no way to add/edit/remove/display such user defined variables in Calligra Tables. Seems oocalc has the same problem.
> * the UI still misses a way to set/modify custom formatings.
> * support for formulas... but this is another beast and partly already covered at bug 283816. I do not plan to work on this anytime soon.
> 
> Update to the first review-request at https://git.reviewboard.kde.org/r/102890/ ;
> * loading of the variables happens now not any longer in the KoTextLoader but in the applications themself right after loading the body-element what is inline with how saving is done.
> * the same logic is applied to Calligra Words, Stage and Tables what means all 3 of them support now loading and saving of user defined variables. Editing can be done direct in the Insert=>Variable=>Custom (should we better rename it to "User defined" rather then "Custom"?) dialog what is inline with how it can be done in OO.org/LO.
> 
> 
> Diffs
> -----
> 
>   libs/kopageapp/KoPADocument.cpp 43e002a 
>   libs/kotext/InsertVariableAction.cpp de68bbf 
>   libs/kotext/KoInlineObject.h fbd1795 
>   libs/kotext/KoVariableManager.h 680a29b 
>   libs/kotext/KoVariableManager.cpp a915b77 
>   libs/kotext/tests/TestKoInlineTextObjectManager.cpp 5bafc82 
>   libs/odf/KoOdfNumberStyles.h 536408d 
>   libs/odf/KoOdfNumberStyles.cpp 5611465 
>   libs/odf/tests/CMakeLists.txt a1c71dc 
>   libs/odf/tests/TestNumberStyle.h PRE-CREATION 
>   libs/odf/tests/TestNumberStyle.cpp PRE-CREATION 
>   plugins/variables/CMakeLists.txt cca8198 
>   plugins/variables/UserVariable.h PRE-CREATION 
>   plugins/variables/UserVariable.cpp PRE-CREATION 
>   plugins/variables/UserVariableFactory.h PRE-CREATION 
>   plugins/variables/UserVariableFactory.cpp PRE-CREATION 
>   plugins/variables/VariablesPlugin.cpp 913aebc 
>   tables/DocBase.h daf758e 
>   tables/DocBase.cpp adb9940 
>   words/part/KWAboutData.h 68e9a6f 
>   words/part/KWOdfLoader.cpp 837280a 
>   words/part/KWOdfWriter.cpp ebfdad9 
> 
> Diff: http://git.reviewboard.kde.org/r/103276/diff/diff
> 
> 
> Testing
> -------
> 
> see the documents attached to bug 282972
> 
> 
> Thanks,
> 
> Sebastian Sauer
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20111128/6a784602/attachment.htm>