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

Do not use global variables #550

Open
lcpz opened this issue Jan 15, 2023 · 3 comments
Open

Do not use global variables #550

lcpz opened this issue Jan 15, 2023 · 3 comments
Milestone

Comments

@lcpz
Copy link
Owner

lcpz commented Jan 15, 2023

The following widgets use _now global variables:

$ git grep "_now" | cut -d: -f1 | uniq                                                                                                                                                                            
alsa.lua
alsabar.lua
bat.lua
contrib/moc.lua
contrib/tp_smapi.lua
cpu.lua
fs.lua
imap.lua
mem.lua
mpd.lua
net.lua
pulse.lua
pulsebar.lua
temp.lua
weather.lua

We should use table-based variables instead, and thus eliminate the use of widget within a settings() function. Example:

local mpd = lain.widget.mpd {
    settings = function()
        local title  = "mpd"
        local artist = "off"

        if mpd.now.state == "play" then -- instead of `mpd_now`
            title  = mpd.now.title
            artist = mpd.now.artist
        end

        mpd.widget:set_text(title .. " " .. artist) -- instead of `widget:set_text`
    end
}

This will break the current usage.

@razamatan
Copy link
Contributor

i can start doing this for the widgets that i actively use (pulsebar, mpd), touched recently for prototyping (weather), and anything else when i feel bored.

as for how this should land wrt #549, i get the sense you want it separated and after it?

razamatan added a commit to razamatan/lain that referenced this issue Jan 20, 2023
- new dependency: needs libsoup gi installation (smaller than gvfs)
- [breaking] weather: `(current/|forecast)_call` changed to `(current|forecast)_uri`
- [breaking] weather: use scoped (non-global) `now.current` and `now.forecast` to avoid `weather_now`
  clobbering.  see lcpz#550 for more details.
- weather: display something useful by default
- weather: render the hour of the forecast by default since the default
  forecast is 5 days by 3h increments
- weather: add degree symbol to temperatures by default
razamatan added a commit to razamatan/lain that referenced this issue Jan 20, 2023
- going forward, there are a couple of ways to access state from
  the settings callbacks
  - `settings` functions should always take in `widget` and `now` as the
    arguments.  so the signature is `settings = function(widget, now)`
  - use `foo.widget` and `foo.now` to access the awesome widget and
    current state (respectively) of `foo` widget
- weather has been done in lcpz#549
- sysload's state is now in a `now` table.  access the 1, 5 and 15 min
  load averages via `now[1]`, `now[5]`, and `now[15]`
- the bat and contrib/tp_smapi widgets have NOT been migrated as i do
  not have a laptop to verify behaviors
razamatan added a commit to razamatan/lain that referenced this issue Jan 20, 2023
- going forward, there are a couple of ways to access state from
  the settings callbacks
  - `settings` functions should always take in `widget` and `now` as the
    arguments.  so the signature is `settings = function(widget, now)`
  - use `foo.widget` and `foo.now` to access the awesome widget and
    current state (respectively) of `foo` widget
- weather has been done in lcpz#549
- sysload's state is now in a `now` table.  access the 1, 5 and 15 min
  load averages via `now[1]`, `now[5]`, and `now[15]`
- the bat and contrib/tp_smapi widgets have NOT been migrated as i do
  not have a laptop to verify behaviors
razamatan added a commit to razamatan/lain that referenced this issue Jan 20, 2023
- new dependency: needs libsoup gi installation (smaller than gvfs)
- [breaking] weather: `(current/|forecast)_call` changed to `(current|forecast)_uri`
- [breaking] weather: use scoped (non-global) `now.current` and `now.forecast` to avoid `weather_now`
  clobbering.  see lcpz#550 for more details.
- weather: display something useful by default
- weather: render the hour of the forecast by default since the default
  forecast is 5 days by 3h increments
- weather: add degree symbol to temperatures by default
razamatan added a commit to razamatan/lain that referenced this issue Jan 20, 2023
- going forward, there are a couple of ways to access state from
  the settings callbacks
  - `settings` functions should always take in `widget` and `now` as the
    arguments.  so the signature is `settings = function(widget, now)`
  - use `foo.widget` and `foo.now` to access the awesome widget and
    current state (respectively) of `foo` widget
- weather has been done in lcpz#549
- sysload's state is now in a `now` table.  access the 1, 5 and 15 min
  load averages via `now[1]`, `now[5]`, and `now[15]`
- the bat and contrib/tp_smapi widgets have NOT been migrated as i do
  not have a laptop to verify behaviors
@razamatan
Copy link
Contributor

PTAL @ https://github.com/razamatan/lain/pull/1/files.

i actually went with the direction of assuming that i would land the libsoup change first, then this one, hence i didn't do a pr yet (and showing the pr diff to address this issue in my own repo). if you feel strongly about flipping the order, i can do the work to put the glob pr before the libsoup one.

lmk.

@razamatan
Copy link
Contributor

everything ok..?

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

No branches or pull requests

2 participants