-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
paneldue: add support for virtual macro folders #342
Conversation
Added support for custom display names and a virtual folder structure. Signed-off-by: Alex <[email protected]>
moonraker/components/paneldue.py
Outdated
@@ -5,6 +5,7 @@ | |||
# This file may be distributed under the terms of the GNU GPLv3 license. | |||
|
|||
from __future__ import annotations | |||
from xmlrpc.client import boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this is an unintentional import, likely pulled in from an annotation below.
@@ -38,12 +39,14 @@ | |||
MIN_EST_TIME = 10. | |||
INITIALIZE_TIMEOUT = 10. | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't submit changes to whitespace in pull requests. Moonraker follows the same standard as Klipper here, code style and whitespace changes aren't accepted unless they are made by the original author of the module.
moonraker/components/paneldue.py
Outdated
self.confirmed_macros: Dict[str, str] = {} | ||
# self.confirmed_macros = { | ||
# "RESTART": "RESTART", | ||
# "FIRMWARE_RESTART": "FIRMWARE_RESTART"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep the RESTART
and FIRMWARE_RESTART
as default options.
moonraker/components/paneldue.py
Outdated
if cmd == "M98": | ||
script = sgc_func(parts) | ||
else: | ||
script = sgc_func(parts[1:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear necessary. In the M98 handler doesn't need parts[0]
, as that is just the command itself.
moonraker/components/paneldue.py
Outdated
@@ -760,6 +773,44 @@ def _run_paneldue_M20(self, arg_p: str, arg_s: int = 0) -> None: | |||
response['files'] = flist | |||
self.write_response(response) | |||
|
|||
def get_macro_dir(self, path: str, issubdir: boolean): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The boolean
annotation is incorrect here, it should be bool
. The return value also needs to be annotated.
moonraker/components/paneldue.py
Outdated
for key, value in self.available_macros.items(): | ||
if value == macro: | ||
cmd = key | ||
logging.info(f"Trying to run macro '{cmd}'...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be debug level logging.
I left some review comments above. At a high level we may need to look into an alternative method of configuration as its possible for macro parameters to contain commas. Also your signed off line needs to contain your full name. Thanks. |
I've modified my working copy with your comments. It' my first Python project so bear with me please ;-). Also, I'm at about 98% of implementing the new object model that's used in firmware 3.3 and newer. Only things to do is multiple chamber heaters/generic heaters and some more testing with different configurations.
True. Was thinking about gcode only and forgot about the Klipper parameters. In my working copy I'm using a '|' now, but if you find this a hacky solution and got another suggestion I can implement that as well. I thought about a list of key/value pairs under macros: but the delimiters cannot be brackets of course (but |
Not a problem. We are a bit limited by the ini file structure here. I think to be reliable we need to set strict requirements on folder names. We could do something like the following:
In this example, folders must be prefixed by "-" and be on one line. We can identify an alias for a macro with an @. Aliases must either contain no whitespace or be quoted. Honestly it might be better to just restrict it to no whitespace. If no alias is available we fall back on the Macro's primary name. To my knowledge macros cant start with a - or @, so we shouldn't have any conflicts. Implementation shouldn't be too difficult, parsing the option may need a recursive call to handle subfolders. With regard to adding support for the new object model, that is ok, however we want that in a different pull request. Also we don't want to break compatibility with the existing model, so either the module needs to autodetect which model to use or it needs to be a config option. |
Thanks. I propose to just make quotes mandatory after a '@' alias. That way a user has the freedom to choose whether to use whitespaces or not. Personally, I like spaces in my display instead of underscores or nameswithoutanything ;-) Would it be acceptable to close this PR and just add this functionality to the version with the new object model and link it to this #95 feature request? BTW, the current version is already fully backwards compatible and will detect what responses to send. I am currently testing with the 3 main versions 1.24, 3.3 and 3.4.0. |
That is fine, I would only ask that changes be separated into commits that clearly define the changes made, ie: a commit for adding virtual folders, a commit for adding 3.x support, etc. I'm guessing that 3.x support is a sizable change. |
Unfortunately, this is not going to work with the current ConfigHelper class (and the underlying RawConfigParser. Your solution is dependent on multi-level parsing for subdirectories and that is not supported (all spaces are stripped in a very early stage (in
This would be parsable while retaining the a folder structure and require no change in the ConfigHelper class. It also has the benefit of using the character that is commonly used for directories. Would this be something you can live with? |
You solution looks good. I agree its more clear for users and easier to parse as it doesn't require recursion. |
Changed the configuration format. Signed-off-by: Alex Bee <[email protected]>
Since it appears that development on this has stalled, I'm closing this for the time being. Thanks. |
Added support for custom macro display names and a virtual folder structure.
Signed-off-by: Alex [email protected]