fix(weather): support multiple widget instances - #490
Open
madhur wants to merge 4 commits into
Open
Conversation
Previously, weather_popup and weather_widget were defined at module level, causing all widget instances to share the same popup and highlight state. This resulted in incorrect behavior when using multiple weather widgets (e.g., for different cities): - Clicking one widget would show another widget's popup - Clicking one widget would highlight another widget in the bar This fix moves weather_popup and weather_widget inside the worker() function, ensuring each widget instance has its own isolated state.
Rename `weather_widget` to `widget` inside worker() to avoid shadowing the module-level `weather_widget` variable on line 43. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Rename `widget` parameter to `w` in update_widget() to avoid shadowing the outer `widget` variable on line 169. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Ryuno-Ki
requested changes
Jan 28, 2026
Ryuno-Ki
left a comment
Ryuno-Ki
left a comment
Collaborator
There was a problem hiding this comment.
As I can see only some renames and a move of some codeblock I vote for rejecting it.
As expected, LLMs like Claude produce low-quality PRs.
| local ICONS_DIR = WIDGET_DIR .. '/icons/' .. icon_pack_name .. '/' | ||
|
|
||
| -- Create popup per widget instance to support multiple weather widgets | ||
| local weather_popup = awful.popup { |
Collaborator
There was a problem hiding this comment.
Why was this moved down?
Collaborator
There was a problem hiding this comment.
I think the goal is to create several popup instead of only one.
This is the real need, this move create scope issue and trigger other renaming.
The renaming is bad, for me this should be improved not rejected.
| } | ||
|
|
||
| local function update_widget(widget, stdout, stderr) | ||
| local function update_widget(w, stdout, stderr) |
Collaborator
There was a problem hiding this comment.
Why was the variable renamed?
Contributor
There was a problem hiding this comment.
I think this is actually an improvement over the actual code.
Renaming the variable here prevents shadowing from the local widget variable from line 169. Making the widget line 169 local makes the main worker function a real constructor/factory/builder that creates new instances of the widget instead of using one shared (that gets overwritten by multiple call of the worker?).
Contributor
Author
|
Let me improve the naming
…On Wed, 28 Jan, 2026, 16:26 Aurélien Lajoie, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In weather-widget/weather.lua
<#490 (comment)>
:
> @@ -152,6 +140,20 @@ local function worker(user_args)
local timeout = args.timeout or 120
local ICONS_DIR = WIDGET_DIR .. '/icons/' .. icon_pack_name .. '/'
+
+ -- Create popup per widget instance to support multiple weather widgets
+ local weather_popup = awful.popup {
I think the goal is to create several popup instead of only one.
This is the real need, this move create scope issue and trigger other
renaming.
The renaming is bad, for me this should be improved not rejected.
—
Reply to this email directly, view it on GitHub
<#490 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD4L57QPCNJBCAD6GGFNJD4JCIXTAVCNFSM6AAAAACTB4Z5TWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTOMJWGA2DKNZSHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Rename parameter 'w' to 'weather_icon' for better readability. This addresses review feedback about unclear variable naming while maintaining the fix for variable shadowing with the outer 'widget'. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes an issue where multiple weather widget instances share the same popup and highlight state, causing incorrect behavior.
Problem:
When using multiple weather widgets (e.g., for different cities), the following issues occur:
Root Cause:
weather_popupandweather_widgetwere defined at module level, so all widget instances created by callingweather_widget({...})shared the same popup and widget reference.Fix:
Move
weather_popupandweather_widgetdeclarations inside theworker()function, ensuring each widget instance has its own isolated state.Test plan
🤖 Generated with Claude Code