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

Release Docking Tools v0.1 (Initial Release) #1466

Merged

Conversation

BenTalagan
Copy link
Contributor

Initial Release for Docking Tools (an action package to resize docks in REAPER).

I have used the "View" category as suggested here by @X-Raym.

@X-Raym
Copy link
Member

X-Raym commented Nov 26, 2024

@BenTalagan
Your dependencies check is a function call, but this will popup reascript error if the function doesn't exist.
First check the presence of the function in the desired table like this for eg:

if not docking or not docking_lib.CheckDependencies or not docking_lib.CheckDependencies() then return end

@BenTalagan
Copy link
Contributor Author

BenTalagan commented Nov 26, 2024

Hi @X-Raym , the docking_lib is provided with the package :

[nomain] talagan_Docking tools/docking_lib.lua

There's no reason it should not be there (?). The function is returned as an export by the library at the end of the docking_lib file, "à la node.js".

@X-Raym
Copy link
Member

X-Raym commented Nov 26, 2024

@BenTalagan as long as it is in another file, all checks have to be made to be sure that file exists. We never know 😉(peopoe may manually copy the script file to another computer, or download ir from github, forgetting the dependencies etc). We still got report of people not installing correctly not using reapack.
It is not too much trouble to add, so it worths it IMHO.

@BenTalagan
Copy link
Contributor Author

Lol ok :-) Well it does not hurt to add those checks ! Making the changes then.

@BenTalagan
Copy link
Contributor Author

BenTalagan commented Nov 26, 2024

Changed my mind : if the docking_lib is not installed, the require statement that tries to load it will fail with an exception. So the check you propose does not seem sufficient for what you want to achieve (which is : making sure everything is installed correctly and not by some copy paste hacking).

I was trying to factorise the dependency check code by putting a helper function in the lib, but if we consider the case were the lib-that-should-be-there is not there this just does not make sense.

The check dependency code should probably copy pasted at the top of each action. I don't see any other way except maybe write a unique action that is duplicated on install and behaves depending on the action title but here it was a bit risky since names are very long and very subject to change... :-)

@X-Raym
Copy link
Member

X-Raym commented Nov 26, 2024

This is one reason I dont use require but dofile after a simple file_exists check ^^

But sure dependency check code has to be put at tip of actions.

Anywqy, if you dont want to bother, we can merge anyway, and wait for future reports (which may never come)

@BenTalagan
Copy link
Contributor Author

@X-Raym , this is pretty interesting to think about the coding guidelines and I'm really open to the discussion :) Thanks for taking the time to review all this ! What do you think of the following flow ?

local ok, docking_lib = pcall(require, "talagan_Docking tools/docking_lib")
if not ok then
  reaper.ShowConsoleMsg("This script is not well installed. Please install it with Reapack.")
  return
end

if not docking_lib.CheckDependencies() then return end

This factorises the dependency check logic, while verifying that the install is correct.

@X-Raym
Copy link
Member

X-Raym commented Nov 26, 2024

using pcall seems a good idea,
try if it works (aka if it pass if is ok, and if the message is displayed if not), and if yes, we can go for it!

Though, better use reaper.MB for warning or errors than console. it is more obvious to the user, and a bit more readable than the console font.

@BenTalagan
Copy link
Contributor Author

Working as expected (and I've switched to MB). Thank you, it was the opportunity to learn a good trick !

@X-Raym
Copy link
Member

X-Raym commented Nov 26, 2024

@BenTalagan You are welcome!

@cfillion does the last commit need a reapack version bump before merging?

@cfillion cfillion merged commit 0049770 into ReaTeam:master Dec 2, 2024
1 check passed
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.

3 participants