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

[Azat Khuzhin]

> On May 13, 2013, 10:48 p.m., Albert Astals Cid wrote:
> > core/textdocumentgenerator.h, line 217
> > <http://git.reviewboard.kde.org/r/109021/diff/3/?file=142972#file142972line217>
> >
> >     This should ideally be a function of the private class
> 
> Azat Khuzhin wrote:
>     Could you explain why you think so?
> 
> Fabio D'Urso wrote:
>     I haven't followed the discussion, but I think I can answer.
>     Because Okular tries to expose as little as possible to external modules, in order to minimize chances of ABI breakage (the compiler doesn't know that initializeGenerator exists so it can't bind your code on it).
> 
> Azat Khuzhin wrote:
>     Sure I can do this, but as far as I know, ABI won't be broken, because here I just add new method, like a new constructor.
> 
> Albert Astals Cid wrote:
>     Actually think about it the other way around. Is there any need for it to be in the public class? If not, shouldn't be there.
> 
> Azat Khuzhin wrote:
>     We have one unresolved issue.
>     
>     I don't wanted to inheriting private class from QObject, do you think it really need to do?
>     Also I suggest to move this into another patch.
> 
> Albert Astals Cid wrote:
>     Why you need the private class to inherit from QObject to make         void initializeGenerator( TextDocumentConverter *converter ); private?
> 
> Albert Astals Cid wrote:
>     Err, i mean to move         void initializeGenerator( TextDocumentConverter *converter ); to the private class, sorry :D
> 
> Azat Khuzhin wrote:
>     Because it has some slots, example addAction:
>     Q_PRIVATE_SLOT( d_func(), void addAction( Action*, int, int ) )
>     
>     And I don't want to call this using Q_Q(), because this add more relation between private/public classes.
>     
>     Even if we do this (drop Q_PRIVATE_SLOT(), and add private slots into private class, and inherit it from QObject),
>     we still have more methods/slots/signals from public class that we must use from private:
>     setFeature()
>     SIGNAL(error())
>     e.t.c.
>     
>     If you ok with that, I have to change it.
> 
> Albert Astals Cid wrote:
>     I think I'm not explaining myself correctly. No worries, i'll do that part myself :-)
> 
> Azat Khuzhin wrote:
>     Seems that I misunderstood you, I will look into final patches in repository for the right explanation.
>     Thanks.
> 
> Albert Astals Cid wrote:
>     Actually, here's what i meant.
>     
>     http://paste.kde.org/~tsdgeos/745328/
>     
>     Clearer now?

Yes, thanks. I did the same in the last patch.


- Azat


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


On May 16, 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 16, 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/20130516/8aaba28a/attachment.html>