-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
base: master
Are you sure you want to change the base?
Conversation
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 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 plus that it can redefine tools and minus that it's CLI only?
- 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 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!
Yes, this sounds a bit like it, except for the absence of a tray icon part.
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. |
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? |
9fb5e65
to
1de33fb
Compare
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:
|
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.
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... :-)
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, |
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.
manpage says "The result must be freed through gdk_rgba_free()." Should be added to paint-context_free() then.
a060a0b
to
a89fd43
Compare
a89fd43
to
048ec6a
Compare
Now all changes requested should be addressed, and the PR be compatible with the current master. Re your question:
|
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.
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. | |||
*/ |
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.
That comment should be moved down to parse_config again.
@@ -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, |
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.
We still have the allocated GdkRGBA here which should be freed.
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.
@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
4e0919b
to
c96bd53
Compare
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:
... I don´t get an eraser, but a red PEN, size 75. I wish I knew more C to contribute, but alas... |
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:
(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.