pisauitab / Visage-LIB-CC

a simpler version of the "Visage" monitor management system for ComputerCraft which can be imported as a library.
GNU General Public License v3.0
0 stars 0 forks source link

`visage.detectMonitors` has "side-effects" #1

Closed fatboychummy closed 2 days ago

fatboychummy commented 1 week ago

Currently, the implementation of visage.detectMonitors has some glaring "side-effects" that will make using it a bit of an annoyance.

`visage.detectMonitors` ```lua function visage.detectMonitors() local mons = {peripheral.find("monitor")} if mons[1] == nil then term.setTextColor(colors.yellow) print("[VisageLib]: No monitors were found.") term.setTextColor(colors.white) else local i = 1 repeat mons[i].setBackgroundColor(colors.black) mons[i].clear() i = i+1 until mons[i] == nil end return mons end ```

The function is named detectMonitors, but it does more than that, and it doesn't seem obvious it would do these things at first.

First, if it finds monitors, it clears all of the monitors. This is a very slight overeach, as the method can be assumed to only detect the monitors, not also change them in some way. It's not the worst though, and I doubt anyone will run into issues specifically because of this, but it might be best to factor it out (or just document that it also clears the monitors!).

Second, it prints to the terminal and sets the terminal color to an arbitrary color afterwards, instead of the old color. If someone is using this library for a GUI program, the writing and setting a color could overwrite whatever they are doing. For example, say the user sets the text color of the terminal to colors.red. This library will change the color to yellow, print the warning message, then set the color to white. The user, assuming that this won't happen, will still assume the text color is red, but now everything is being drawn in white, with an offset position since print moved the cursor.

I recommend just outright removing the warning. The user writing code using this library can detect if mons[1] is nil, and go from there.

Revised `detectMonitors` ```lua function visage.detectMonitors() return {peripheral.find("monitor")} end ``` Alternatively, use ```lua table.pack(peripheral.find("monitor")) ``` This adds to the table a `.n` value that can be referenced for the amount of monitors that have been detected, i.e: ```lua -- some_program.lua local mons = visage.detectMonitors() if mons.n == 0 then error("No monitors attached!", 0) end ```

As a small aside while I am reviewing the code here, running monitor methods in parallel is unnecessary, as they do not yield. You can just use a for loop in setOnAll, as it will achieve the same thing in the same amount of time -- the code will just be a lot simpler to manage.

Revised `setOnAll` ```lua function visage.setOnAll(mons, fun) for i, mon in ipairs(mons) do if mon ~= "moved" then fun(mon, i) end end end ```
pisauitab commented 1 week ago

oh wow, I never imagined to get a code review, thank you for the info, I'll work on updating the code as soon as I can and about the parallel programs, the original idea was to let the user be able to run separate programs on each monitor, and I am still trying to figure it out. Thanks again for the help