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

Manual color fetch feature and fixes on dynamic wallpaper compatibility #242

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

discojapi
Copy link

@discojapi discojapi commented Dec 29, 2024

Added 2 new features, a "Fetch colors" button in the widget, which fetches colors from current wallpaper, and the manual fetch option, which basically freezes the wallpaper detection protocol in order to always need to press the fetch button to get new colors. These new options are useful if the user is using a dynamic wallpaper or video, and also partially fixes the color select bug addressed at Issue #229 .
Edit: Add images
Screenshot 1
Screenshot 2

@luisbocanegra
Copy link
Owner

@discojapi thanks for your contribution, will take a look at the changes later today.

@luisbocanegra
Copy link
Owner

Hi again, tried running your changes and it seems to work, but sometimes does not use the current wallpaper but the last one. I also think the main loop should be rewritten into something less troublesome, I am just not sure how.

I think, for now at least, would be better to implement this differently, here is how I imagine it:

  • Back-end exposes "run once" flag
  • widget creates/reads manual_fetch config option, back-end may ignore it

Back-end runs with "run once" flag

  1. Terminates any existing instances utils.kill_existing()
  2. Runs once
  3. Exits

From the widget:

  1. When manual color fetch is checked (manual_fetch === true):
    1. Stops back-end in case it is running kde-material-you-colors --stop
    2. Enables fetch colors button
    3. Disables back-end monitoring timer and "Back-end is not running" Kirigami.InlineMessage
  2. When fetch colors is clicked
    1. Runs back-end with "run once" flag

This would have the advantage of the back-end not needing to be running all the time

@discojapi
Copy link
Author

discojapi commented Jan 3, 2025

Hi again, tried running your changes and it seems to work, but sometimes does not use the current wallpaper but the last one. I also think the main loop should be rewritten into something less troublesome, I am just not sure how.

I think, for now at least, would be better to implement this differently, here is how I imagine it:

* Back-end exposes "run once" flag

* widget creates/reads `manual_fetch` config option, back-end may ignore it

Back-end runs with "run once" flag

1. Terminates any existing instances `utils.kill_existing()`

2. Runs once

3. Exits

From the widget:

1. When manual color fetch is checked (`manual_fetch === true`):
   
   1. Stops back-end in case it is running `kde-material-you-colors --stop`
   2. Enables fetch colors button
   3. Disables back-end monitoring timer and "Back-end is not running" `Kirigami.InlineMessage`

2. When fetch colors is clicked
   
   1. Runs back-end with "run once" flag

This would have the advantage of the back-end not needing to be running all the time

I can see how these changes would save some resources and make the code a little more readable, but it is worth enough? The time of response would double or even triple since the backend would set everything up in every fetch.

@luisbocanegra
Copy link
Owner

I can see how these changes would save some resources and make the code a little more readable, but it is worth enough? The time of response would double or even triple since the backend would set everything up in every fetch.

That's a good point, your approach makes more sense in that case, after all if someone is using the widget they most likely expect it to be always running and reactive.

Comment on lines +754 to +773
// Button to manually fetch the colors on screen //
Timer {
id: fetchTimer
interval: settings.main_loop_delay * 1000; running: false; repeat: false;
onTriggered: settings.fetch_colors = false
}
RowLayout {
PlasmaComponents3.Button {
text: "Fetch colors"
icon.name: 'refreshstructure'
onClicked: {
settings.fetch_colors = true
fetchTimer.start()
}
PlasmaComponents3.ToolTip {
text: "Manually fetch the colors on the current wallpaper"
}
}
}

Copy link
Owner

@luisbocanegra luisbocanegra Jan 3, 2025

Choose a reason for hiding this comment

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

I think it would be better to put this button next to the "Select color" label. Also hide the button if manual fetch is disabled

--- a/src/plasmoid/package/contents/ui/FullRepresentation.qml
+++ b/src/plasmoid/package/contents/ui/FullRepresentation.qml
@@ -751,26 +751,6 @@ ColumnLayout {
 
                             }
 
-                            // Button to manually fetch the colors on screen //
-                            Timer {
-                                id: fetchTimer
-                                interval: settings.main_loop_delay * 1000; running: false; repeat: false;
-                                onTriggered: settings.fetch_colors = false
-                            }
-                            RowLayout {
-                                PlasmaComponents3.Button {
-                                    text: "Fetch colors"
-                                    icon.name: 'refreshstructure'
-                                    onClicked: {
-                                        settings.fetch_colors = true
-                                        fetchTimer.start()
-                                    }
-                                    PlasmaComponents3.ToolTip {
-                                        text: "Manually fetch the colors on the current wallpaper"
-                                    }
-                                }
-                            }
-
                             RowLayout {
                                 PlasmaComponents3.Label {
                                     text: "From Wallpaper"
@@ -833,17 +813,38 @@ ColumnLayout {
                             // Color selection
                             RowLayout {
                                 Layout.preferredWidth: mainLayout.width
-
-                                PlasmaComponents3.Label {
-                                    text: "Select color"
-                                    id:selectColorLabel
-                                    Layout.fillWidth: settings.color!==""
+                                RowLayout {
+                                    id: selectColorLayout
+                                    Layout.fillWidth: true
+                                    PlasmaComponents3.Label {
+                                        text: "Select color"
+                                    }
+                                    // Button to manually fetch the colors on screen //
+                                    Timer {
+                                        id: fetchTimer
+                                        interval: settings.main_loop_delay * 1000; running: false; repeat: false;
+                                        onTriggered: settings.fetch_colors = false
+                                    }
+                                    RowLayout {
+                                        visible: settings.manual_fetch
+                                        PlasmaComponents3.Button {
+                                            text: "Fetch colors"
+                                            icon.name: 'refreshstructure'
+                                            onClicked: {
+                                                settings.fetch_colors = true
+                                                fetchTimer.start()
+                                            }
+                                            PlasmaComponents3.ToolTip {
+                                                text: "Manually fetch the colors on the current wallpaper"
+                                            }
+                                        }
+                                    }
                                 }
 
                                 // Single color picker when color is not empty
                                 Components.CustomColorButton { // Components.Custom
                                     id: colorButton
-                                    Layout.alignment: Qt.AlignHCenter
+                                    Layout.alignment : Qt.AlignRight
                                     visible: settings.color!==""
                                     showAlphaChannel: false
                                     dialogTitle: "Choose source color"
@@ -861,7 +862,7 @@ ColumnLayout {
                                 GridLayout { //PlasmaComponents3.ScrollView
                                     property var gridSpacing: Kirigami.Units.mediumSpacing
                                     visible: settings.color===""
-                                    columns: Math.floor((mainLayout.width - selectColorLabel.width) / (
+                                    columns: Math.floor((mainLayout.width - selectColorLayout.width) / (
                                         controlHeight * .75 + gridSpacing))
                                     rowSpacing: gridSpacing
                                     columnSpacing: gridSpacing

@luisbocanegra
Copy link
Owner

Another thing, could you please rewrite the commit history so there is no partial changes and fixes to your own changes? This helps keeping a clean history.

Also, recently I started following the conventional commit standard, so new commits should follow that too (forgot to update the guidelines), like you did for the first commit 👍.

@luisbocanegra luisbocanegra linked an issue Jan 3, 2025 that may be closed by this pull request
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.

Manual color fetch
2 participants