Review Request 113302: Static KEncodingFileDialog in staging/kio

[Kevin Ottens]

> On Oct. 20, 2013, 9:32 a.m., David Faure wrote:
> > OK, I'm just wondering if it still makes sense to make it static-methods-only.
> > It can still very well work the other way, which can allow extra flexibility if needed (e.g. for kate to add another widget into it, or use it non-modal, etc.). What's the reason for making it less flexible? Just the fact that nobody uses the constructor right now? (I assume you checked that?).
> > 
> > Kévin?
> 
> Kevin Ottens wrote:
>     Initially I think I proposed even a namespace. :-)
>     
>     But seeing the current solution I think you're right, we could indeed allow instances to be created.
> 
> Teo Mrnjavac wrote:
>     Ok, but that would add some more duplication over what KFileDialog already does. Afaict, the only non-static usage of KEncodingFileDialog was in KMail's messagecomposer. I had checked with kdepim people a while ago, and they have no problem with KEncodingFileDialog becoming static only in KF5.

I'm not sure which kind of duplication you have in mind, but surely just making the ctor/dtor pair public and adding a result() public method would be enough (said method you could use in your static methods BTW reducing some of the code duplication between them).


- Kevin


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


On Oct. 19, 2013, 11:30 a.m., Teo Mrnjavac wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113302/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2013, 11:30 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Kevin Ottens.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> This makes KEncodingFileDialog static, moves it to staging/kio and moves some stuff from KFileDialog to KFileWidget to reduce duplication.
> 
> 
> Diffs
> -----
> 
>   kio/CMakeLists.txt ac80e393c877280dd8033ec13e8e772afea6d2f9 
>   kio/kfile/kencodingfiledialog.h abb939abeb000126005f1a1a9c6fd50b8bd322bd 
>   kio/kfile/kencodingfiledialog.cpp d55d908473aae2859838d88fd776cc65cecf3317 
>   kio/kfile/kfiledialog.cpp 32eb98a4490a31c5ed51150ca3cb7af5375dc67e 
>   staging/kio/src/filewidgets/CMakeLists.txt e8d8c2edf11ad6352e13eb6e7436f828a4a55e3a 
>   staging/kio/src/filewidgets/kencodingfiledialog.cpp PRE-CREATION 
>   staging/kio/src/filewidgets/kfilewidget.h f7b162ab39b997274b21f9eff0c6374545e0a9ad 
>   staging/kio/src/filewidgets/kfilewidget.cpp 824ac563f3f8c463426f0a45e792f107ac402a40 
> 
> Diff: http://git.reviewboard.kde.org/r/113302/diff/
> 
> 
> Testing
> -------
> 
> Seems to work fine in a test app.
> 
> 
> Thanks,
> 
> Teo Mrnjavac
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20131021/5fd66df5/attachment.html>