Filelight into kdereview

[Friedrich W. H. Kossebau]

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?

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).

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

"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?

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.

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

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

    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>
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.

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

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()

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_dialog.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_dialog.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')

SettingsDialog::SettingsDialog(): use KIcon if setting the buttons, not 
SmallIcon
SettingsDialog::addFolder(): KUrl("/") should use QDir::rootPath() again
SettingsDialog::canvasIsDirty(int): should emit enum values, not magic numbers

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)

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 :)

Cheers
Friedrich
-- 
Okteta - KDE Hex Editor - http://utils.kde.org/projects/okteta