Haruna in kdereview

[George Florea Banus]

Formatting of my first reply looks terrible in the web mailing list.
Hopefully this one is better. Sorry.

On 20.07.2021 00:39, Albert Astals Cid wrote:
> El dilluns, 19 de juliol de 2021, a les 14:54:26 (CEST), George Florea 
> Banus va escriure:
> You need a
>
> KLocalizedString::setApplicationDomain("haruna")
>
> just after creating your QApplication. Otherwise translations are not 
> going to be loaded.

Done.

> The menus look&feel is a bit broken (separators and highlight being 
> drawn past the actual menu width, icons being painted "blurrily"), but 
> i guess that's not really your fault.

Nothing done here.

> The Setting has a "Help!" <- remove the !

Done.

> in that "Help!" menu you have a KHelpCenter entry that should be 
> called "Haruna handbook" like in the rest of applications.

Done.

> The first time i open the app the "Color scheme" combo box doesn't 
> have contents, given there's a default entry i'd expect to see that.

I encountered a weird issues here. The color scheme is saved to the 
"ColorScheme" entry 
https://invent.kde.org/multimedia/haruna/-/blob/master/src/settings/generalsettings.kcfg#L46 

But the default value for "GeneralSettings::colorScheme()" is 
BreezeLight, even if I set a default value.
If I rename the entry, for example to "ColorScheme123" then it gets the 
correct default value.
Or if I move it to another settings class like MouseSettings, then it 
also gets the correct default value.

> The "english only" help is kind of weird. Why are you using html for 
> that? Seems like something you should do with tooltips and if those 
> QML components don't support tooltips, at least make the help page 
> with QML so it's translatable?

That's what I used before adding the docbook file. The content of the 
html pages are the same as the content in the docbook.
I kept them because the KHelpCenter doesn't render the html correctly.
Sometimes there's no space between words, if rows are long enough they 
get behind the scrollbar, there's no bold.
As for why not use tooltips, sometimes there's just to much text. 
https://invent.kde.org/multimedia/haruna/-/blob/master/help/VideoSettings.html

> I've no idea what the "Open config..." entries in the settings menu 
> are supposed to do, but both fail with "file doesn't exist" for me.

They open the config file and config folder, but it tried to open a 
wrong path, I fixed that now.
And now it's also hidden when the config folder doesn't exist.

> Esc not only exits fullscreen (good) but enters fullscreen (weird).

Fixed.

> The "Primary Subtitle" MenuItem that tries to be a title is kind of 
> weird because it has exactly the same look&feel that the other menus, 
> at least disable clicking on it, because i can click on it and then 
> it's even more confusing if it's really wants to be a title or not 
> without reading the code :D

Yes, it's used as title. I wanted to change that menu, but couldn't come 
with anything better.
I think I'll change it so it manages only the primary subtitle.

> It does remember the last thing i was playing, which I dislike, I 
> tried to look for a config option to disable it but doesn't seem to be 
> one. Feel free to ignore this.

I'll add an option to disable that behavior, someone else requested it too.

> The thing you call Header in View -> "Hide header" is called 
> historically called toolbar, i think you should rename it.

I'll called it header because that's what Qt calls it and the footer is 
also a toolbar.
Do you want me to change only user facing part or  everything? What 
about the footer?

> All in all, the app is nice, but IMHO suffers a bit from being QML 
> based and not being able to use KXMLGUI.
>
> Cheers,
>    Albert

Thanks for the review.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20210720/08c7bf42/attachment-0001.htm>