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

Add option to change tools on the command line #189

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pascal-niklaus
Copy link
Contributor

This refers to #187

Changes:

(1) Command-line option added that allows changes of the tools while gromit is running, following the syntax of the config file.
Example:

  gromit-mpx --change-tool '"default"=PEN(color="green" size=4 arrowsize=2)'

(2) Updated man page to include this option, and also added documentation for --line that was missing

(3) Implementation details: a new file parser.c/.h was created that contains functions to scan the pen definitions. These functions are now called from config.c and from the callback invoked when changing these setting on-the-fly when gromit is running.

Copy link
Owner

@bk138 bk138 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some comments at the respective code lines and some general thoughts.

The general ones:

gromit-mpx.1 Outdated Show resolved Hide resolved
src/callbacks.c Outdated Show resolved Hide resolved
src/parser.c Outdated Show resolved Hide resolved
@pascal-niklaus
Copy link
Contributor Author

I have some comments at the respective code lines and some general thoughts.

The general ones:

* So this is essentially the meta-tool called toolswitcher devised in [Make the various tools selectable by the system tray icon and from control #91](https://github.com/bk138/gromit-mpx/issues/91) plus that it can redefine tools and minus that it's CLI only?

Yes, this sounds a bit like it, except for the absence of a tray icon part.

* My general goal is to make the app easy for non-pro users to use, so [Add UI for creating/editing tools and mapping to buttons and/or maybe keys #110](https://github.com/bk138/gromit-mpx/issues/110) is the go-to solution for a user interface; also the app should be usable w/o any third-party tools like key-mappers and the like. So, _in general_, I would not add too much special CLI features _unless_ they're small and easy to maintain. So I guess I would merge this if you can slim it down (removing the refacto into parse.c e.g.), otherwise rather not. In any case, please don't be put off, I very much appreciate any contribution!

I can easily move the content of parser.c into config.c, and call these functions from main.c for the command-line option. That part is straightforward.

Additional thoughts:

One of my use cases is with a graphics tablet and presentations in full-screen mode, with no tray icon visible. With extra tools (colors, rect, line, single and double arrows, etc) at some stage the number of modifier keys is quickly exhausted.

Key mappings would work, but then it would be great to be able to distinguish keystrokes from an external keyboard (like a numeric keypad for tool changes) and the "normal" keyboard. That would be along the line with the option to have two mouse cursors, one for gromit (driven by e.g. a tablet pen) and another one for regular work.

@bk138
Copy link
Owner

bk138 commented Mar 2, 2024

Regarding this one, I think a CLI toolswitcher is something we would like to have.

Not sure if a tool-redefiner would add too much extra complexity. I'd rather have one single place (the .cfg or later on, the .ini file after #110) where definitions are stored and read from for switching/drawing purposes.

@pascal-niklaus
Copy link
Contributor Author

Regarding this one, I think a CLI toolswitcher is something we would like to have.

Not sure if a tool-redefiner would add too much extra complexity. I'd rather have one single place (the .cfg or later on, the .ini file after #110) where definitions are stored and read from for switching/drawing purposes.

Another way to think about this is that the diversity of tools that gromit offers will keep growing (at least I have some more ideas in that direction ;-) ). Given this, I think it would be good to allow changing the individual attributes of the tools separately (e.g. tool type, color, line width etc).

For example, I may wish to draw in yellow, and then change from a PEN to a RECT or LINE (all still yellow). Later, maybe I want to change the color to green, or just modify the line width (again, preserving these choices for the future tools).

With individual tool definitions, an enormous variety of tools would need to be defined to accommodate all combinations (and one would have to remember which is which).

I understand that this is a different approach than providing a set of a few carefully chosen predefined tools, like this is now implemented with the modifier keys.

For the CLI, my suggestion is therefore to provide an option to change the individual attributes of the default tool.

This is based on my experience with a screenshot annotation tool I frequently use, ksnip. ksnip allows changing tools with a single key press (e.g. L = line, A= arrow, D = double arrow, R = rect etc), which in principle allows for a very smooth workflow. However, in each and every tool the color and line width has to be set separately, which IMO spoils the user experience.

But maybe the two approaches could be combined? For example, some tools could be defined in the .ini file and selected by their number, and the default tool attributes changed as outlined above?

@pascal-niklaus
Copy link
Contributor Author

I have now integrated the content of "parser.c/parser.h" into config.c/config.h, as suggested.

Now, two command-line options are available for tool changes:

  1. Using --change-tool or -T, a complete tool definition can be changed, for example like so:
gromit-mpx --change-tool '"default"=PEN(color="Limegreen" size=4 arrowsize=2 arrowtype="end")'
  1. Using --change-attribute or -A an individual attribute of a tool can be changed, keeping all others. This way, it for example is possible to change from PEN to LINE and keep color, width, arrows etc, or to just change the color and line width, regardless of the tool. Examples are
gromit-mpx --change-attribute '"default"=(color="Limegreen" size=4)'   # change just attributes keeping tool
gromit-mpx --change-attribute '"default"=PEN(color="cyan")'            # change tool and some attributes
gromit-mpx --change-attribute '"default"=RECT'                         # change just the tool

@pascal-niklaus pascal-niklaus requested a review from bk138 March 19, 2024 14:06
Copy link
Owner

@bk138 bk138 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some thinking, I'm OK with the tool definition on the CLI.

Not sure about the attribute changer, is it really that useful that it justifies extra code, i.e. isnt't that already covered by the tool redefinition functionality?

Also, am a bit afraid of the vast refactor of config.[ch], but I guess this is needed to be able to parse config from other places in the program as well... :-)

gromit-mpx.1 Outdated Show resolved Hide resolved
gromit-mpx.1 Show resolved Hide resolved
gromit-mpx.1 Show resolved Hide resolved
src/callbacks.c Show resolved Hide resolved
src/main.c Outdated
@@ -775,10 +773,10 @@ void setup_main_app (GromitData *data, int argc, char ** argv)
data->modified = 0;

data->default_pen =
paint_context_new (data, GROMIT_PEN, data->red, 7,
paint_context_new (data, GROMIT_PEN, gdk_rgba_copy(data->red), 7,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manpage says "The result must be freed through gdk_rgba_free()." Should be added to paint-context_free() then.

@bk138 bk138 assigned pascal-niklaus and unassigned bk138 Mar 30, 2024
@pascal-niklaus pascal-niklaus force-pushed the devel_change_tool branch 4 times, most recently from a060a0b to a89fd43 Compare March 31, 2024 08:54
@pascal-niklaus
Copy link
Contributor Author

pascal-niklaus commented Mar 31, 2024

Now all changes requested should be addressed, and the PR be compatible with the current master.

Re your question:

  • code refactoring in config: I agree that moving the config code to these extra functions looks like a relatively big change. OTOH, the code largely is the same, and IMO it makes the main config function much more readable (the loops in there were very long before).

  • about changing the attributes of tools: this is very little extra code since it builds on the same functions. And I think it allows to mirror the "non-digital" drawing experience better. For example, one changes to a pen with a different color or width, and then draws a line, or a rectangle. With just complete tool changes, the rectangle would for example always be yellow and have thick lines, and the line be red. Binding color changes and tool changes to different keystrokes (in the desktop environment or in some other ways) seems to allow for a more natural drawing experience, IMO. For example, one could switch to yellow, and then draw lines, rectangles etc, and then change to green and do the same. Or use rectangles and then just change the colors.

@pascal-niklaus pascal-niklaus requested a review from bk138 March 31, 2024 09:08
Copy link
Owner

@bk138 bk138 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much - almost there!

@@ -33,9 +33,34 @@
Select and parse system or user .cfg file.
Returns TRUE if something got parsed successfully, FALSE otherwise.
*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment should be moved down to parse_config again.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -141,10 +141,9 @@ void on_monitors_changed ( GdkScreen *screen,

parse_config(data); // also calls paint_context_new() :-(


data->default_pen = paint_context_new (data, GROMIT_PEN, data->red, 7, 0, GROMIT_ARROW_END,
data->default_pen = paint_context_new (data, GROMIT_PEN, gdk_rgba_copy(data->red), 7, 0, GROMIT_ARROW_END,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have the allocated GdkRGBA here which should be freed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pascal-niklaus what do you think about those?

…ment in config.h, free default colors when exiting, add missing snap minlen maxangle simplify to callbacks
@LucaBoschetto
Copy link

Hi and thank you both for this wonderful tool.

I am using @pascal-niklaus's branch devel-change-tool because that's exactly what I need for my remote classes.

One problem I found is that if, for example, I have the default set as per the original config (thus a red PEN) and I issue the command:

gromit-mpx -T '"default" = ERASER (size = 75)'

... I don´t get an eraser, but a red PEN, size 75.

I wish I knew more C to contribute, but alas...

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