Filelight into kdereview

[Friedrich W. H. Kossebau]

Hi,

Vendredi, le 29 janvier 2010, à 01:52, Friedrich W. H. Kossebau a écrit:
> Hi Martin,
> 
> this is the kdeutils coordinator writing :)
> 
> I give you... some points... to do, sorry ;)
> 
> Jeudi, le 28 janvier 2010, à 22:55, Martin T. Sandsmark a écrit:
> > Hi!
> >
> > I've just moved Filelight and documentation into kdereview.
> >
> > Afterwards, I would like it to be included in KDE-utils. If not, I'll
> > move it back to extragear (where the KDE 3 version was).
> 
> I agree with Aaron, would make a good addition to the kdeutils, no
>  objection from me.
> 
> > So, anyone have any comments, objections.
> 
> Just did a short test of program and kpart, also a quick code scan.
>  Filelight is a nice program, I have done and will make use of it, thanks
>  for working on it. :)
> 
> But seems there is some work left:
> 
> GUI:
> The circle around the end of the label arrow looks like it is off by 1,1
> pixels, or?

Fixed.

> It's not obvious to me what the value in the middle of the chart means.
>  Should it be the total size of the current dir? Then it seems there is a
>  mistake in the update of the display, because it doesn't always match
>  (yes, seems so, as can be seen after calling Rescan, where it then shows
>  the current size).

Fixed.

> Pressing Up, Back or Forward should also cancel a scan for the current dir,
> no?

Not fixed for the program filelight.

And the Stop button does not cancel the scanning for the KPart in Konqueror.

> "RadialMap View" as name for the KPart should be changed in something along
> the lines of the Dolphin views, which are "Icons", "Details", "Columns".
> Perhaps "Radial Map" or something which includes the file size aspect?

Fixed.

> The KPart's ui can't use "options_configure" as this while end up in the
>  menu with the text "Configure NameOfShell", which will for one lead to two
>  times the same entry for most programs (see e.g. Konqueror's Settings
>  menu) and also be wrong.

Not fixed.

> Choosing "KDE colors" gave me all parts in the same grey. And perhaps it
> should be better "System colors"?

Not fixed.

> Code:
> Why the -fPIC in filelight/CMakeLists.txt?
> add_definitions(${QT_DEFINITIONS} ${KDE4_DEFINITIONS} -fPIC)

Fixed.

>     setXMLFile("filelightui.rc");
>     setXMLFile("filelight_partui.rc");
> can be both removed, as $APPNAMEui.rc is the default ui.rc name.
> 
> Includes are best sorted, with first KDE, than Qt, than standard libs. Also
> best use <QtModule/QClass>

Fixed.

> For portability: "/" would be better QDir::rootPath() and qgetenv("HOME")
> QDir::homePath(), or? Similar MainWindow::slotComboScan() might be improved
>  to use QDir::homePath().
> 
> File::humanReadableSize(...) does a word puzzle. This should be changed to
>  use KLocale::formatByteSize() or similar.

Not fixed.

> The "BIG FAT TODO TODO TODO" in LocalLister::readMounts() which asks to
>  "Use Solid for this" would be nice to be done soonish, indeed ;)

Not fixed.

> Part::createAboutData(): The name of the part should be rather
> "filelight_part" or "filelightpart", not just "filelight". Use the same
>  name as for the part's module (like done with K_EXPORT_PLUGIN and in the
>  CMakeLists.txt), also adapt the part's ui.rc name to filelightpartui.rc,
>  then.
> 
> Part::mapChanged(): word puzzle, no i18n()

Not fixed.

> Showing the dialog this debug output is generated:
> Object::connect: No such slot QWidget::toggleScanAcrossMounts(bool) in
> /home/koder/Kode/kdesvn/trunk/kdereview/build.debug/filelight/src/part/ui_d
> ialog.h:299 Object::connect:  (sender name:   'scanAcrossMounts')
> Object::connect:  (receiver name: 'Dialog')
> Object::connect: No such slot QWidget::toggleDontScanRemoteMounts(bool) in
> /home/koder/Kode/kdesvn/trunk/kdereview/build.debug/filelight/src/part/ui_d
> ialog.h:300 Object::connect:  (sender name:   'dontScanRemoteMounts')
> Object::connect:  (receiver name: 'Dialog')
> Object::connect: No such slot QWidget::toggleDontScanRemovableMedia(bool)
>  in
>  /home/koder/Kode/kdesvn/trunk/kdereview/build.debug/filelight/src/part/ui_
> dialog.h:301 Object::connect:  (sender name:   'dontScanRemovableMedia')
> Object::connect:  (receiver name: 'Dialog')

Not fixed.

> SettingsDialog::SettingsDialog(): use KIcon if setting the buttons, not
> SmallIcon

Fixed.

> SettingsDialog::addFolder(): KUrl("/") should use QDir::rootPath() again

Fixed.

> SettingsDialog::canvasIsDirty(int): should emit enum values, not magic
>  numbers

Not fixed.

> RadialMap::Widget::mousePressEvent(): Hardcoding of Konqueror and Konsole
>  is a no-go, please use the KDE classes which offer abstract start of the
>  system's assigned handlers (don't remember right now which one this is,
>  need to ask somebody else)

Not fixed.

> Having not looked at the docs yet, there might be other people better at
> judging them :)
> 
> Feature wishes:
> Tooltip would be closer to Dolphin's one, with icon and preview.
> Names of radia lables could have icon at the start, but I agree that there
>  is few space often.
> 
> So, nothing grave, but would like to see most of the above fixed, before it
> moves over to kdeutils. Other than that it seems well done :)

Hm, now the delayed deadline has passed, too. Too many things not fixed. And 
still no reply from you, Martin. 

So IMHO you should move filelight back to playground, until you find the time 
to fix the other problems and also be available to the discussion of the 
review :)
Anyone disagrees with this treatment?

Will move it myself if nothing happens until 3rd March 8 pm CET.

Cheers
Friedrich
-- 
KDE Okteta - a simple hex editor - http://utils.kde.org/projects/okteta