liquidshell in kdereview

[Martin Koller]

Hi,

thanks for you review!

On Samstag, 4. November 2017 19:41:27 CET Kai Uwe Broulik wrote:
> Hi,
> 
> AppMenu.cxx:
> * You can use NoDotAndDotDot flag in entryInfoList() to already skip those

I just want to skip ".." - but yes, I can use QDir::AllEntries | QDir::NoDotDot

> * You seem to be using KFileItem and the url you generate just for isDesktopFile(), you could use KDesktopFile::isDesktopFile(), also the isDir() check could come before, a folder cannot be a desktop file

That does not work. KDesktopFile::isDesktopFile() does not open the file but just checks the file extension.
I do have desktop files without extension.
The url is used for starting the desktop file (see the lambda function at the bottom of AppMenu::fill())

> Battery.cxx:
> * It's using Solid/Power API that has never been released (and probably never will be..)

Interesting.
On openSuse 42.2 it is included (released) in the solid-devel package.
On what is "it was never released" based ?
But I have no problem to use something else, if there is a way to know if the AC Power is plugged or not
(and get a signal when it changes).
What can I use instead ?

> * you assume remainingTime() == -1 to be "when on AC", I'm not sure this assumption holds

I don't know either. I think this was based on try/error.
Maybe with an alternative to Solid/Power I can change that.

> * secsToHM looks not ideal form an i18n perspective

Suggestion ?
 
> CMakeLists.txt:
> * No project name set

fixed

> ConfigureDesktopDialog.cxx:
> * instead of QOverload or old syntax you can static_cast<void(KUrlRequester::*)(const QString &)>(&KUrlRequester::returnPressed)

Thanks for the hint, but this is really ugly to write.
I see no advantage in doing so. 

> ConfigureDesktopDialog.hxx:
> * Reason for *returning* const &?

You mean this ?
    const DesktopWidget::Wallpaper &getWallpaper() const { return wallpaper; }

Of course. What else ?

> desktop.cxx:
> * Missing AA_UseHighDpiPixmaps attribute

fixed

> DesktopWidget.cxx:
> * window type NET::Desktop should imply NET::KeepBelow

ok, removed

> * The list of applets is completely hardcoded and makes assumptions in the class (for "Weather" create WeatherApplet)
> instead of using some plugin/introspection mechanism

Yes, by intention.
I have currently no plans to have a plugin mechanism

> LockLogout.cxx:
> * Use QDBus instead of calling dbus-send command-line tool or xdg-screensaver

Because ?

> QuickLaunch.cxx:
> * Similar issues as AppMenu.cxx

filter now on QDir::Files

> * Internet browser assumes kde.org is start page

yes. And ?
This is a default entry if the user did not configure anything.
And I don't know how to run the "default web browser" than using KRun() with an http url ...

> StartMenu.cxx:
> * Similar issues as LockLogout.cxx

you mean system() instead QDBus ?
Do you think there might be distributions not shipping the dbus-send command ?
If so, I should change that, else - who cares.

> SysTray.cxx:
> * The fill function is also awfully hardcoded

hardcoded is the part for the features I supply.
Why do you think this is awful ?
 
> General (UI) observations:
> * Qt scales it quite well for high-dpi, however the application doesn't announce high dpi support leading to blurry icons, especially tray icons

since I have no hardware to test that, I did not care.
However I have now added the AA_UseHighDpiPixmaps attribute

> * The CPU indicator in the panel cannot be disabled and is distracting

What I want is a system which is usually idle - which is one reason why I implemented liquidshell in the first place.
On my system, I normally don't see any CPU bar at all. And if I see one, I go and check who's the culprit.

> * There's lots of I18N substitution errors all over the place (I18N_ARGUMENT_MISSING)

how do you see this ? I don't know what to look for or change.

> * The "device notifier" lists *all* devices (not just removables)

That should not happen, since the code filters only on removable devices.
See DeviceList::addDevice():

  // show only removable devices
  if ( device.is<Solid::StorageDrive>() &&
       !device.as<Solid::StorageDrive>()->isRemovable() )
  {
    //qDebug() << device.udi() << "not Removable";
    return;
  }

  // show only removable devices
  if ( device.parent().is<Solid::StorageDrive>() &&
       !device.parent().as<Solid::StorageDrive>()->isRemovable() )
  {
    //qDebug() << device.parent().udi() << "parent() not Removable";
    return;
  }

so which devices do you get which should not appear ?
Or do you know a better way to check for removables only ?

> and isn't scrollable if there are lots of devices,

I can easily add a QScrollArea like in the NotificationList, but I think you usually should not have
that many removable devices which would justify this.
Maybe it's the problem that there are devices shown which should not.
Let's find out which ones.

> network also doesn't seem scrollable

yes, it isn't. But I also think you will not have that many entries shown.
Am I wrong ?

> * No battery applet,

I don't understand.
You'd like to have a battery desktop applet ?
That's of course doable if needed (I just had no need for that).

> icon just opens KCM

More importantly: It shows the current battery status and has a tooltip.
What do you mean by "just" ?

> * "Bluetooth is not operational", why show the icon then

Because otherwise you would not be able to turn it on (a click opens the kcm)

> * Notifications doesnt handle multiple incoming ones well (just stacks dialogs ontop of each other)

interesting use case. never had problems with that, but that can be changed, of course.

> * Start button breaks Fitt's law (I cannot open it by janking the cursor into the lower-left corner)

I can live with that (BTW: in plasmashell you can't either)

> * No multi-screen support whatsoever

commit a03d66c69d78c6eec9f0728c38a597ea874e3787
Author: Martin Koller <kollix at aon.at>
Date:   Sat Nov 4 13:50:46 2017 +0100

    handle multi-screen setup

    Desktop Panel always only on primary screen,
    Wallpaper drawn per screen in corresponding scaled version


> * I like the background color selector combination of ComboBox and "custom"

thanks, but kudos go to KDE's KColorCombo

> * Often uses QString instead of enums

You probably mean the Weather Applet (units) where this is done since the selected units is used as string
in the URL to be queried.

> * Task Bar does not announce "iconified geometries" to KWin thus minimize animations go to the wrong place (centre of screen usually)

Interesting point. How would I do this ?
(Since openGL does not work on my machine (crappy Intel driver) - therefore also the need to have a widgets based solution -
I don't use animations ...)

> * The coding style and naming practises do not follow "modern" KDE Frameworks guidelines

It's the coding style and naming practises I use for the last 25 years, and liquidshell is no framework lib.

-- 
Best regards/Schöne Grüße

Martin
A: Because it breaks the logical sequence of discussion
Q: Why is top posting bad?

()  ascii ribbon campaign - against html e-mail 
/\                        - against proprietary attachments

Geschenkideen, Accessoires, Seifen, Kulinarisches: www.lillehus.at