Review Request: Move KNewMenu from libkonq to kdelibs.

[David Faure]

> On 2010-01-15 22:45:40, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kfile/knameandurlinputdialog.h, line 46
> > <http://reviewboard.kde.org/r/2629/diff/1/?file=17171#file17171line46>
> >
> >     should the parent have a default of 0 like other QObject classes to to?
> >     
> >     similar for the other members, do they all need values or can they have defaults? since setSuggestedName and setSuggestedUrl already exist to do this, the ctor could even be stripped back to just KNameAndUrlInputDialog(QWidget *parent = 0)?

Toplevel widgets should really be given a parent, ask Lubos about kde3 horror stories ;)
I see that KFileDialog has QWidget *parent without a default value, for instance.

About nameLabel and urlLabel: these are the text for the *labels*, like "Enter a name:" and "Enter a URL:". Don't confuse with setSuggestedName/setSuggestedUrl which are the values for the lineedits next to these labels. Of course I could still turn these into setters, for more readable app code. Requires more member vars, but that's no big deal.


> On 2010-01-15 22:45:40, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kfile/knameandurlinputdialog.h, lines 56-60
> > <http://reviewboard.kde.org/r/2629/diff/1/?file=17171#file17171line56>
> >
> >     looking at the api i expected a getter; and then i thought maybe name() would be that getter.
> >     
> >     perhaps setCurrentName(const QString &name) and QString currentName() const would be clearer API? (similar for setSuggestedUrl below?)

I see little reason for a getter, actually ;)
The standard reason for such getters would be "so that we can add a Q_PROPERTY and use the stuff in designer", but in this case it's a dialog, so not really useable in designer anyway.


> On 2010-01-15 22:45:40, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kfile/knewfilemenu.h, line 65
> > <http://reviewboard.kde.org/r/2629/diff/1/?file=17173#file17173line65>
> >
> >     parentWidget should be the last parameter? can it be defaulted to null?
> >     
> >     personally, i'd change parent to actionCollection and parentWidget to just parent; more in-line with existing API

Very true about the naming (current naming comes from kde3 semantics). Fixed.
Arg order changed too, since the collection and name belong together.

Not convinced about null default value here either (for the QWidget*); it's also used for any dialogs popped up by the class, so better have a parent widget pointer for proper window stacking.


> On 2010-01-15 22:45:40, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kfile/knewfilemenu.h, lines 80-91
> > <http://reviewboard.kde.org/r/2629/diff/1/?file=17173#file17173line80>
> >
> >     getters?

What for? :-))

I'm a bit torn about that question in general. When designing a class I find myself wondering why anyone would ever need to get back the stuff they just set into the class; while when using existing classes I sometimes find myself in lack of a getter, due to some piece of code not knowing what another piece of code did :-) Or for a derived class, I guess. OK that convinces me at least for popupFiles(), could be used in slotResult to know which files this was about. Added.


> On 2010-01-15 22:45:40, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kfile/knewfilemenu.h, line 99
> > <http://reviewboard.kde.org/r/2629/diff/1/?file=17173#file17173line99>
> >
> >     should "slot" be removed from its name?

OK, makes sense, especially since one should call it directly in the common case.


> On 2010-01-15 22:45:40, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kfile/knewfilemenu.h, line 128
> > <http://reviewboard.kde.org/r/2629/diff/1/?file=17173#file17173line128>
> >
> >     should slot be remove from its name? perhaps jobResult(KJob *job)?

Hmm. Many virtual slots in KIO jobs are called slotResult, and more importantly, if I rename this one, I'm in trouble in the libkonq kept-for-BC subclass of KNewFileMenu.


- David


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


On 2010-01-15 21:09:00, David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2629/
> -----------------------------------------------------------
> 
> (Updated 2010-01-15 21:09:00)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Move KNewMenu from libkonq to kdelibs/kfile, so that http://reviewboard.kde.org/r/2535/ can use it (in order to add "RMB / New File" in kdiroperator, which benefits KFileDialog and other users of kdiroperator).
> 
> Since libkonq is public API, the "moving" needs a renaming (and the existing classes will use the moved ones), so the new class name is KNewFileMenu. The dialog for entering name+url is moved too, as KNameAndUrlInputDialog [should this go to libkio instead? better decide now or we'll need to change the name again].
> 
> The good thing is that this gets rid of some code duplication for creating directories: the libkonq code moving to libkfile means that KDirOperator::mkdir can now use it, rather than its own fork of it. Done in this patch, so as a side effect this patch actually adds RMB / Create New in kdiroperator - taken from 2535 :) I'll let Abhishek finish it though (adding the support for "only showing the mimetypes that are relevant to the current application").
> 
> 
> This addresses bug 93230.
>     https://bugs.kde.org/show_bug.cgi?id=93230
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kfile/CMakeLists.txt 1071368 
>   /trunk/KDE/kdelibs/kfile/kdiroperator.h 1071368 
>   /trunk/KDE/kdelibs/kfile/kdiroperator.cpp 1071368 
>   /trunk/KDE/kdelibs/kfile/knameandurlinputdialog.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kfile/knameandurlinputdialog.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kfile/knewfilemenu.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kfile/knewfilemenu.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/2629/diff
> 
> 
> Testing
> -------
> 
> RMB / Create New in KFileDialog. Creating a dir still enters it like it did before.
> 
> 
> Thanks,
> 
> David
> 
>