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

Background-Wallpaper #22

Merged
merged 2 commits into from
Jun 2, 2021
Merged

Background-Wallpaper #22

merged 2 commits into from
Jun 2, 2021

Conversation

zMoooooritz
Copy link
Contributor

Contains the changes described in #21

@zMoooooritz
Copy link
Contributor Author

This is a basic implementation that allows to set a background image and alpha blend the "normal" icons on top.
Currently each widget is updated each second and every image is newly alpha blended.

Since this is a computational overhead I thought about adding some code that allows to update the widget-image only once at LoadDeck or every X milliseconds (according to the type of widget).
This would be required anyway for possible future internet-based widgets, for example widget_weather (you dont want to fetch that data every second) or so on.

@muesli what are your thoughts on these changes?

@zMoooooritz zMoooooritz force-pushed the background branch 3 times, most recently from b4ea944 to 548e405 Compare June 1, 2021 20:08
@zMoooooritz zMoooooritz requested a review from muesli June 1, 2021 20:13
@muesli
Copy link
Owner

muesli commented Jun 1, 2021

Just a heads up, I'll take the liberty to play with this a bit and potentially simplify a few things.

I like the idea of custom update intervals, but I think this should probably be a separate PR, as I can already see a few more cleanups/changes coming along with it. I may drop this commit from the branch, but we can easily rescue it and move it to a separate PR 😃

@zMoooooritz
Copy link
Contributor Author

Yes definetly 👍 , I'm sorry for this "raw" code but I'm just starting to wrap my head around the way go works.

I would have created two PRs but then thought to add it right away since the bare implementation does increase the cpu footprint by up to 100%.

Whether it stays in this PR or gets moved here are some things that should be thought about:

  1. updateWidgets does not update widget 4 in case widget 3 has not finished its update -> call these updates in a asynchronous way
  2. it could be desireable to specify the update interval per widget in the .deck-files
  3. there was one more thing ... I'll add it later when it comes to mind again

@muesli
Copy link
Owner

muesli commented Jun 2, 2021

Yes definetly +1 , I'm sorry for this "raw" code but I'm just starting to wrap my head around the way go works.

Nothing to apologize for, I can already see you picking up a few Go idioms, and your code improving!

I would have created two PRs but then thought to add it right away since the bare implementation does increase the cpu footprint by up to 100%.

Understood! I have guarded the background image from being repainted unnecessarily in my latest changes. Let me know if I missed something and you can still see an increased CPU footprint.

Whether it stays in this PR or gets moved here are some things that should be thought about:

1. `updateWidgets` does not update widget 4 in case widget 3 has not finished its update -> call these updates in a asynchronous way

We need to be careful tho, as the communication with the device itself is strictly synchronous.

2. it could be desireable to specify the update interval per widget in the .deck-files

Agreed!

3. there was one more thing ... I'll add it later when it comes to mind again

😆

@muesli
Copy link
Owner

muesli commented Jun 2, 2021

Heads up: I removed the separate Background config struct, as its only purpose was storing a string anyway. Did you expect to add other settings to this struct?

Comment on lines +46 to +48
if err != nil {
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not certain if that is what you wanted to achieve with these changes. But in case you define a button without an icon (not impossible) this results in the following not much saying error message (and deckmaster exits):
2021/06/02 18:17:44 error: open : no such file or directory

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch! This is indeed just a side-effect of the missing error handling. I'll address this in a separate PR, which will improve the rendering in a few ways.

@zMoooooritz
Copy link
Contributor Author

Heads up: I removed the separate Background config struct, as its only purpose was storing a string anyway. Did you expect to add other settings to this struct?

Ohh yes I think I hade something planned there and aimed for a clean config files ... but yes without additional settings this is better

@muesli
Copy link
Owner

muesli commented Jun 2, 2021

Ohh yes I think I hade something planned there and aimed for a clean config files ... but yes without additional settings this is better

If you can already envision some additional settings right now, we may as well keep it in a separate Background struct, otherwise we'll just break compatibility with previous config files in the near future.

@zMoooooritz
Copy link
Contributor Author

zMoooooritz commented Jun 2, 2021

Ohh yes I think I hade something planned there and aimed for a clean config files ... but yes without additional settings this is better

If you can already envision some additional settings right now, we may as well keep it in a separate Background struct, otherwise we'll just break compatibility with previous config files in the near future.

The motivation of this PR was mainly to get rid of the need to jump into your favourite image editor and create custom buttons (with background).
With the current implementation you are required to crop the background exactly to the specified dimensions.
One setting could therefore be how the image (with any dimensions) is supposed to be resized and aligned on the streamdeck.

@zMoooooritz
Copy link
Contributor Author

zMoooooritz commented Jun 2, 2021

Nothing to apologize for, I can already see you picking up a few Go idioms, and your code improving!

Thank you!

Understood! I have guarded the background image from being repainted unnecessarily in my latest changes. Let me know if I missed something and you can still see an increased CPU footprint.

Looks great! Love the way u adapted the changes in deck.go and widget.go.

We need to be careful tho, as the communication with the device itself is strictly synchronous.

Is this the reason spamming an 'empty' button does block the update of other widgets? 🤔

@muesli muesli added the enhancement New feature or request label Jun 2, 2021
@muesli
Copy link
Owner

muesli commented Jun 2, 2021

Is this the reason spamming an 'empty' button does block the update of other widgets? thinking

Not quite. Check out the select statement at https://github.com/muesli/deckmaster/blob/master/main.go#L141:

This will cause the program to sleep and wait until one of its cases below occur:

  • the time.After call times out: we update the widgets
  • a key gets pressed (<-kch): we handle the key press
  • a window gets activated/closed (<-tch): we update the recent-window widget

If you keep spamming a button, the timer never gets a chance to timeout, as it gets restarted after each key event.

@muesli muesli merged commit 61809b6 into muesli:master Jun 2, 2021
@zMoooooritz zMoooooritz deleted the background branch June 2, 2021 17:46
@muesli
Copy link
Owner

muesli commented Jun 3, 2021

Forgot to drop a big "thank you" for your contribution here: great stuff, thank you @zMoooooritz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants