[dict] Modernize configuration window
ClosedPublic

Authored by filipf on Apr 24 2019, 10:06 PM.

Details

Summary

This patch bumps dictionary's settings QtQuickControls from version 1 to 2.

It also changes the configuration category's icon to match the applet's one.

NOTE: It would be good if we could get keyboard navigation to work, it doesn't seem to want to be doing it now.
Test Plan

Before:

After:

Diff Detail

Repository
R114 Plasma Addons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
filipf created this revision.Apr 24 2019, 10:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 24 2019, 10:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.Apr 24 2019, 10:06 PM
ngraham requested changes to this revision.Apr 24 2019, 10:14 PM

Why doesn't mouse wheel scrolling with with ScrollView? That seems odd and unexpected. I would recommend we use that component and figure out why it doesn't work. Also it seems semantically incorrect to be putting a ScrollablePageinside a page. That thing is supposed to be the whole page, not one component of it.

BTW, since while using ScrollView you'll probably run into this, to make a QQC2 ScrollView display its frame, you do this:

QQC2.ScrollView {
    Component.onCompleted: background.visible = true;
    [blabla]
}

Also LOL we really need to fix that broken icon: https://bugs.kde.org/show_bug.cgi?id=399568

This revision now requires changes to proceed.Apr 24 2019, 10:14 PM

Why doesn't mouse wheel scrolling with with ScrollView?

Not sure, but will investigate further.

Another issue with the QQC2 ScrollView is that it's also kind of inferior visually :/ . The labels are glued to the frame and the scrollbar just floats on top of everything:

ngraham added a comment.EditedApr 24 2019, 11:41 PM

You can add internal padding yourself, and the scrollbar floating above everything is an implementation issue IIRC

filipf updated this revision to Diff 57114.Sat, Apr 27, 7:12 PM

make it work with ScrollView and make ScrollView not buggy by using ContentWidth and ContentHeight to allow mouse scrolling, as well as setting clip to true so the scrollbar is actually contained within the view

filipf edited the summary of this revision. (Show Details)Sat, Apr 27, 7:14 PM
filipf edited the test plan for this revision. (Show Details)
davidedmundson accepted this revision.Sat, Apr 27, 11:39 PM
filipf planned changes to this revision.Sun, Apr 28, 12:33 AM

Err sorry, this is a binding loop monstrosity, I cannot be defining the parent with the children and vice versa at the same time.

filipf updated this revision to Diff 57121.Sun, Apr 28, 1:01 AM

remove binding loop

Is there some trick to applying this? I don't actually see any change to the dictionary widget's configuration window. I got the same problem when I started working on this myself, so it's not something you did wrong...

Is there some trick to applying this? I don't actually see any change to the dictionary widget's configuration window. I got the same problem when I started working on this myself, so it's not something you did wrong...

Works for me both in the latest Manjaro stable and in KDE Neon unstable. I've been editing /usr/share/plasma/plasmoids/org.kde.plasma_applet_dict/contents/ui/ConfigDictionaries.qml

filipf updated this revision to Diff 57555.Sat, May 4, 4:33 PM

change string "Providers:" to "Provider:"

filipf updated this revision to Diff 57556.Sat, May 4, 4:34 PM

wrong diff, revert

ngraham accepted this revision.Mon, May 6, 9:43 PM
ngraham added subscribers: broulik, mart.

@broulik or @mart, any idea how we get Keyboard navigation working here? The Tab key just goes straight to the buttons and ignores the dictionary list view. Maybe a FocusScope would help?

This revision is now accepted and ready to land.Mon, May 6, 9:43 PM
mart requested changes to this revision.Thu, May 9, 2:11 PM
mart added inline comments.
applets/dict/package/contents/ui/ConfigDictionaries.qml
49–50

Control {

which should have the highlight rectangle as background:

and the Label as contentItem:

this will give it some nice paddings, as now it looks quite cramped

56

Kirigami.Theme.highlightColor

60

Label {

64

Kirigami.Theme.textColor

This revision now requires changes to proceed.Thu, May 9, 2:11 PM
GB_2 added a subscriber: GB_2.Thu, May 9, 3:22 PM
GB_2 added inline comments.
applets/dict/package/contents/ui/ConfigDictionaries.qml
32–33

This can be removed when you chang the the other color properties.

filipf updated this revision to Diff 57813.Thu, May 9, 6:35 PM
  • use Kirigami.Theme for color, remove syspal
  • replace ListView Item with Control and have it be the delegate
  • replace Text with Label and have it be ListView's contentItem
NOTE: keyboard navigation still not working; I can have the ScrollView focusable by tabbing by seting FocusPolicy, but that doesn't help too much
filipf marked 5 inline comments as done.Thu, May 9, 6:36 PM
filipf added inline comments.
applets/dict/package/contents/ui/ConfigDictionaries.qml
49–50

looks much better, thanks :)

filipf edited the test plan for this revision. (Show Details)Thu, May 9, 6:39 PM
mart accepted this revision.Tue, May 14, 3:50 PM
This revision is now accepted and ready to land.Tue, May 14, 3:50 PM
This revision was automatically updated to reflect the committed changes.
filipf marked an inline comment as done.