Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rely on QQC2 styles instead of internal themes #108

Merged
merged 19 commits into from Jun 29, 2022
Merged

Conversation

dobey
Copy link
Contributor

@dobey dobey commented Mar 9, 2022

This is the set of changes necessary to have the keyboard styled entirely by the used QQC2 style rather than internal themes.

@dobey
Copy link
Contributor Author

dobey commented Mar 11, 2022

Here's a screenshot of how the changes here currently look on a Plasma Mobile session on pinephone:

Maliit QQC2

(edited: Updated screenshot with no more sticky highlights on keys, and contrasting background for space key)

@KAMiKAZOW
Copy link
Member

Looks amazing

@dobey
Copy link
Contributor Author

dobey commented Mar 14, 2022

This is mostly working pretty well now. However, there is a couple of issues with specific styles, particularly with the Breezez style running under X11, as either it or Kirigami (or kiconthemes) seems to prefer the icon theme as named in Xsettings rather than what is specified by the keyboard with QIcon::setThemeName(), but seemingly only for some icons, so there can be a weird mix of icons in some situations on X11 with that style. And Suru style seems to have a few oddly sized icons in some situations.

So I'm not sure this is yet "ready" to merge. I'm not sure if there's any similar issue under Wayland either.

@KAMiKAZOW
Copy link
Member

We could start to use the Milestones feature for releases, then merge this and immediately open an issue about the icon themes and assign that milestone so the issue must be fixed before the next release

@dobey
Copy link
Contributor Author

dobey commented Mar 15, 2022

We could start to use the Milestones feature for releases, then merge this and immediately open an issue about the icon themes and assign that milestone so the issue must be fixed before the next release

Well, I want to make another release, before merging this in any case. And I don't think the icons issue can be fixed on our side anyway, without providing our own set of icons we use everywhere, instead of using named icons from the theme.

@awhiemstra
Copy link
Contributor

On Plasma Desktop, this needs to use QApplication rather than QGuiApplication to ensure qqc2-desktop-style loads correctly, since that uses QtWidgets for the actual control look. If you use QGuiApplication, the fusion style will be used which looks rather bad.

@awhiemstra
Copy link
Contributor

Another problem is that the custom mouse handling on top of the ToolButton in CharKey means that the ToolButton doesn't properly change state which means there's no clear pressed state for the key.

@dobey
Copy link
Contributor Author

dobey commented Mar 25, 2022

If you use QGuiApplication, the fusion style will be used which looks rather bad.

This doesn't seem to be the case for me. Running with QT_QUICK_CONTROLS_STYLE=org.kde.desktop seems to give the correct and expected result, with the desktop style.

@dobey
Copy link
Contributor Author

dobey commented Mar 25, 2022

Another problem is that the custom mouse handling on top of the ToolButton in CharKey means that the ToolButton doesn't properly change state which means there's no clear pressed state for the key.

I'm not sure there's anything we can do about that, though. There doesn't seem to be any way to propagate events from a MultiPointTouchArea to the button underneath. And simply calling pressed() and released() on the button breaks the keyboard.

@dobey dobey force-pushed the drop-themes branch 3 times, most recently from 8a569ac to 22e9a94 Compare April 7, 2022 21:49
@dobey dobey marked this pull request as ready for review June 24, 2022 20:25
@dobey
Copy link
Contributor Author

dobey commented Jun 24, 2022

I think it's time to get this in as well. I've been testing it a bit again on my PinePhone Pro, and it's working pretty great there. I seem to only have the icon coloring issue when running under GNOME on X11, which resulted in my attempts at resolving it via https://invent.kde.org/frameworks/kirigami/-/merge_requests/511 in kirigami.

If some could test this on Plasma Desktop as well, especially on both Wayland and X11, that would be very appreciated.

@dobey dobey mentioned this pull request Jun 27, 2022
@dobey dobey force-pushed the drop-themes branch 2 times, most recently from 8629c10 to aae00d0 Compare June 27, 2022 18:40
jpetersen
jpetersen previously approved these changes Jun 29, 2022
Copy link
Member

@jpetersen jpetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we find issues with that change we still can fix it. But it is a good change in general.

dobey added 10 commits June 29, 2022 16:41
Use a hard coded value for border color where it should be shown, which is
already the same color used in all existing themes. Also set the border width
to 1 pixel for all borders.
Ensure we get the background from the qqc2 style in use by using a Page
component as the main surface for the Keyboard.
This simplifies the keys a bit by moving the icon properties into CharKey from
ActionKey, and removing the color properties, using an ToolButton to show
icons on keys that use them. In addition, this also makes the keyboard look a
bit more modern.
These were the only two icon themes used by the internal themes, so we need to
still use them as appropriate, since qqc2 styles themselves don't provide the
icons used. Other themes also don't provide the icons we need yet. Ideally,
we should just provide all the icons we need ourselves, in a neutral style, so
that we don't need to rely on any such themes, but some qqc2 styles which use
the default QQuickIconLayout implementation don't always render icons loaded
from a qrc resource.
Some keys have annotations that are quite difficult to discern in some
situations, so make the label for the annotation a little larger to more
easily read them in those cases.
@dobey dobey merged commit fa554a6 into maliit:master Jun 29, 2022
@dobey dobey deleted the drop-themes branch June 29, 2022 20:56
@IlyaBizyaev
Copy link

Does this require the theme to be forced somewhere? Built Maliit 2.3.1 from source and managed to get it working with Plasma (5.25.90), but it looks like it's styled with QQC2's "default" style:

Screenshot_20220917_182625

@dobey
Copy link
Contributor Author

dobey commented Sep 17, 2022

Does this require the theme to be forced somewhere? Built Maliit 2.3.1 from source and managed to get it working with Plasma (5.25.90), but it looks like it's styled with QQC2's "default" style

See #159 as the issue is due to Plasma style not working automatically for applications that don't use QGuiApplication.

JIaxyga pushed a commit to sm7150-mainline/pmaports that referenced this pull request Mar 10, 2024
…ove kscreenlocker override back to plasma-mobile (MR 4882)

A bunch of override files in postmarketos-base-ui-plasma seem to have come from the plasma-mobile ui package, which probably shouldn't be on desktop and bigscreen.

With Plasma 6, a lot of the overrides are now no longer necessary for Plasma Mobile.

Situation with each file:
- 000-gschema.override - This likely was for changing maliit themes and GTK title bar layout for Plasma Mobile, maliit theming was dropped (maliit/keyboard#108), and title bar layout is now handled within Plasma Mobile
- kdeglobals - This was for setting the look-and-feel on Plasma Mobile and default browser, it's now handled within Plasma Mobile envmanager
- kwinrc - These were settings for disabling window decorations and setting the vkbd to maliit, which is now handled in Plasma Mobile envmanager
- kscreenlockerrc - This is to autolock the screen after login (tinydm autologs in), the file was for Plasma Mobile, I don't think we want it for Desktop since SDDM is used there
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants