[Okular-devel] Review Request 109021: Font selector for TextDocumentGenerator

[Albert Astals Cid]

> On May 8, 2013, 6:02 p.m., Albert Astals Cid wrote:
> > core/textdocumentsettings.h, line 29
> > <http://git.reviewboard.kde.org/r/109021/diff/2/?file=142647#file142647line29>
> >
> >     We need to install this class so generators outside the okular repo can use it. This means you need to add it to the install list somewhere in the CMakeLists.txt and that you should add a d-pointer so it's possible to keep Binary Compatibility when we need to add more fields, etc. Also some documentation at least mentioning an @since is needed
> 
> Azat Khuzhin wrote:
>     It is already in install()
>     
>     https://git.reviewboard.kde.org/r/109021/diff/2/?file=142647#file142647line29
>     
>     What about private class - I agree, will be done.
> 
> Azat Khuzhin wrote:
>     I think here we must add map of widgets/objects into TextDocumentSettings/TextDocumentSettingsSkeleton
>     To allow manually adding fields from generators.
>     
>     Don't you think so?
> 
> Azat Khuzhin wrote:
>     Also we can avoid of private classes, if we did like this.

You pointed me to a line that contains no install() call, i'm confused.

My idea about adding fields in generators is just calling generalSettings()/generalSettingsWidgets() and adding there the needed stuff.

How does your solution work?


> On May 8, 2013, 6:02 p.m., Albert Astals Cid wrote:
> > core/textdocumentgenerator.cpp, line 217
> > <http://git.reviewboard.kde.org/r/109021/diff/2/?file=142645#file142645line217>
> >
> >     We are leaking the settings, no?
> >     
> >     Also i think it'd make more sense for the skeleton parent to be "this" and not "parent", otherwise you are parenting it to the grandfather
> 
> Azat Khuzhin wrote:
>     About TextDocumentSettings, it's an interesting question.
>     
>     I can delete it normally if "Configure Backend" dialog didn't opens, but if you open it, after closing okular, there will be segmentation fault.
>     KConfigDialog::addPage() something doing with settings object, don't have enough time to investigate this.
>     
>     Maybe you have ideas?

The dialog may be taking ownership of the widget, giving it a parent is an easy solution to avoid the leak


- Albert


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


On May 12, 2013, 10:02 p.m., Azat Khuzhin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109021/
> -----------------------------------------------------------
> 
> (Updated May 12, 2013, 10:02 p.m.)
> 
> 
> Review request for Okular, Albert Astals Cid and Eike Hein.
> 
> 
> Description
> -------
> 
> Development history:
> https://github.com/azat/okular/compare/master...font-selector-for-plain-text-formats
> 
> http://quickgit.kde.org/?p=clones%2Fokular%2Fazatkhuzhin%2Fokular.git&a=commitdiff&h=font-selector-for-plain-text-formats&hp=master
> (but it has some delay, about a day or so while this clone will be updated, maybe because of mirrors?)
> 
> Link to thread from mailing list:
> http://comments.gmane.org/gmane.comp.kde.devel.okular/13279
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 543e6df 
>   conf/textdocumentsettings.ui PRE-CREATION 
>   core/textdocumentgenerator.h dd75c5c 
>   core/textdocumentgenerator.cpp f370ded 
>   core/textdocumentgenerator_p.h 749d6f2 
>   core/textdocumentsettings.h PRE-CREATION 
>   core/textdocumentsettings.cpp PRE-CREATION 
>   core/textdocumentsettings_p.h PRE-CREATION 
>   generators/epub/CMakeLists.txt f076ed9 
>   generators/epub/generator_epub.h cef2879 
>   generators/epub/generator_epub.cpp 59bb2bf 
>   generators/epub/libokularGenerator_epub.desktop 5c853a3 
>   generators/fictionbook/generator_fb.h a898397 
>   generators/fictionbook/generator_fb.cpp 2317083 
>   generators/fictionbook/libokularGenerator_fb.desktop 099268c 
>   generators/ooo/converter.cpp 1124e2a 
>   generators/ooo/generator_ooo.h 3441c7a 
>   generators/ooo/generator_ooo.cpp 793ee58 
>   generators/ooo/libokularGenerator_ooo.desktop 328ae26 
>   generators/txt/CMakeLists.txt 5a126b7 
>   generators/txt/generator_txt.h 5c15ec4 
>   generators/txt/generator_txt.cpp 93ca4aa 
>   generators/txt/libokularGenerator_txt.desktop 235e23d 
> 
> Diff: http://git.reviewboard.kde.org/r/109021/diff/
> 
> 
> Testing
> -------
> 
> Tested manually
> 
> 
> Thanks,
> 
> Azat Khuzhin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20130513/10606944/attachment-0001.html>