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

Keyboard navigation/patching #869

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

Conversation

HenriAugusto
Copy link
Contributor

@HenriAugusto HenriAugusto commented Jan 15, 2020

This is a prototype branch that allows you to fully navigate and connect your patches with the keyboard. Here is what's new:

Features

  • Navigate through objects in/outlets and connections with the keyboard
    • mod+arrows
  • New objects are placed accordingly to selected in/outlet
  • Enter/exit editing the text of an object
    • mod+Return
  • "goto" - select any object in the patch
    • mod+g
  • Create connections with the keyboard
    • magnetic connector: create a connection in ascending order of distance
      • c and shift+c
    • digit connector: press a key from 0 to 9 to connect to any of the 10 closest in/outlets
      • mod+shift+arrow
    • Connect Object dialog
      • mod+shift+arrow twice
  • Simulate a click in an object with the keyboard
    • mod+Return
    • used to trigger [bng] and [tgl]
    • used to open subpatches
    • used to trigger |message boxes(
  • Show object indexes
    • mod+i
    • useful for the "Go To" and "Connect Objects" dialog
    • may be useful for dynamic patching (unnoficial)
  • Help patch in the docs
    • doc/7.stuff/keyboard.navigation/keyboard.patching-help.pd
  • enable/disable keyboard navigation in the edit menu

I've tried to keep it simple and intuitive. Most of the navigation is done with mod+arrow

Testing is needed in all platforms (I'm testing in Win10 and building with Msys2/MinGW as suggested in (INSTALL.TXT)

TODO

  • adjust canvas console hotkey or alternative re-use the Ctrl+M to a dedicated dialog or remove this feature from the core and maybe create a gui-plugin (removed)
  • "goto" should have a dedicated embedded dialog with numboxes.
    • I've kept the name "goto" instead of "select" because of mod+g. As expected, mod+s is aptly taken for "save"
  • "kbdconnect" should have a dedicated embedded dialog with numboxes.
  • update documentation
    • move kbd nav docs to 7.stuff/patching/ (instead of 7.stuff/keyboard.navigation
  • improve magnetic connector so it doesn't encourage bad fan-outs
    • magnetic connector always works with a single connection (c) and creates fan-outs if you hold Shift
  • allow the user to enable/disable keyboard navigation (currently in the Edit menu)
  • implement a generic tooltip system
    • (obj indices and in/outlet indices are going to use this system)
  • modify the kbdnav code to better integrate on the core - [x] do the keybindings in the gui side
    • there is still code that i don't know how to put in a separate file. See this comment in the part Kbdnav outisde g_kbdnav.c
    • Remove #ifdefs

Ideas to discuss

  • allow user to create an object not connected to anything with a hotkey.
  • allow multi digit numbers instead of the "Connect Objects" dialog.
  • currently you can use the "Go To" and "Connect Object" dialogs from the edit menu even if kbdnav is disabled. Also disable those entries VS leave them them usable while kbdnav is deactivated?
  • To keep or not to keep the old Tab binding for object traversal
  • be able to change number atoms with the keyboard (and maybe sliders and radios)
  • be able to resize objects with the keyboard
    • mod+shift+left/right

PD kbdnav demo 27 may 2020

Most of the code related to kbdnav is on g_kbdnav.c but in a few cases
we will need to include related functionality in other c files.

In g_kbdnav.h We define "HAVE_KEYBOARDNAV" to allow conditional
compiling of the kbdnav stuff.
main modifications:
- stuff related to t_editor_private
- canvas_key redirect key presses to be handled in g_kbdnav.c
- canvas_goto()
- canvas_objindexes (to be called from the gui)
1. If needed, we display each object index when canvas_map() is called
2. We call the kbdnav routine that draw the connections so we can
   highlight the selected one
kbdnav stuff mainly responsible for creating new obj/msgbox/atoms
in the correct position and connecting them to the correct in/outlet.

Also there is kbdnav_iopos() which provides a single function to get
in/outlet positions.

Currently multiple functions of the pd core use copy/pasted
routines to deal with that. I intend to unify them to use this new
function later.
When kbdnav is active this allow the blue in/outlet selection rectangle
to be drawn for those objects
In this version this is an "tcl entry" that the user can activate with
shift+' (resulting in a doublequote).

Any message you type there is sent to the patch (just like in dynamic
patching).

 The only exception is "connect" which the code translates to
 "connect_with_undo".
@HenriAugusto HenriAugusto changed the title Kbdnav Keyboard navigation/patching Jan 15, 2020
@HenriAugusto
Copy link
Contributor Author

HenriAugusto commented Jan 15, 2020

Currently build tests are failing. Maybe it is a problem with the #define HAVE_KEYBOARDNAV conditional compilation because the tests fail due to linking problems. Compiles normally for me on Win7 with MinGW (GCC). Did i miss some compile flag?

@porres
Copy link
Contributor

porres commented Jan 15, 2020

I like the idea of showing index of objects cause it is also useful for dynamic patching (knowing the index to make connections)

@porres
Copy link
Contributor

porres commented Jan 16, 2020

  • control + arrows not really working on macOS
  • I don't think that emulating a bang is that useful for GUIs, like shift+return on a bang

Ctrl+Arrows are captured by the OS so we need to use CMD as our main
modifier on OSX.

We still need to think of a shortcut for show obj indexes because
Cmd+tab is also used by the OS.
@HenriAugusto
Copy link
Contributor Author

ctrl+arrows were being capture by the OS so now Cmd is the kbdnav modifier on OSX.

Still we should change Cmd+Tab for displaying obj indexes.

I'm thinking Cmd+i on OSX and Ctrl+i on Win.

src/g_editor.c Outdated Show resolved Hide resolved
src/g_editor.c Outdated Show resolved Hide resolved
src/g_editor.c Outdated Show resolved Hide resolved
src/g_editor.c Outdated Show resolved Hide resolved
src/g_editor.c Outdated Show resolved Hide resolved
@Spacechild1
Copy link
Contributor

Cool stuff! Still have to test. There are some opportunities to not have any keyboardnav specific code in existing methods at all, other than function calls wrapped in #ifdefs. I gave some hints in my comments. Maybe you can try and see how far you get?

Copy link
Contributor

@umlaeute umlaeute left a comment

Choose a reason for hiding this comment

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

while this is certainly an improvement, the actual problem why the header-file cannot be found by the travis-ci builds is that it are not included in the Makefile.am. you need to add g_kbdnav.hto the noinst_HEADERS

Copy link
Contributor

@umlaeute umlaeute left a comment

Choose a reason for hiding this comment

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

and regarding the failing AppVeyor-builds: you should also update the alternative-buildsystem in:

  • src/makefile.gnu
  • src/makefile.mac
  • src/makefile.mingw
  • src/makefile.msvc

@umlaeute umlaeute self-requested a review January 18, 2020 22:10
src/Makefile.am Show resolved Hide resolved
src/g_editor.c Outdated Show resolved Hide resolved
src/g_bang.c Show resolved Hide resolved
@@ -687,6 +693,11 @@ void canvas_map(t_canvas *x, t_floatarg f)
if (x->gl_isgraph && x->gl_goprect)
canvas_drawredrect(x, 1);
sys_vgui("pdtk_canvas_getscroll .x%lx.c\n", x);
#ifdef HAVE_KEYBOARDNAV
t_kbdnav *kbdnav = canvas_get_kbdnav(x);
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to conform to C89 (sic!) and declare your variables at the beginning of the code-block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay!

src/g_editor.c Outdated
if(editor)
{
t_editor_private *private = editor->e_privatedata;
t_kbdnav *kbdnav = &private->kbdnav;
Copy link
Contributor

Choose a reason for hiding this comment

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

*always use canvas_get_kbdnav() instead of directly accessing the è_privatedata`.
(there are multiple occurences of this anti-pattern in the code)

src/g_kbdnav.h Outdated Show resolved Hide resolved
src/g_kbdnav.h Outdated Show resolved Hide resolved
tcl/pd_bindings.tcl Outdated Show resolved Hide resolved
# on X11, <Shift-Tab> is a different key by the name 'ISO_Left_Tab'...
# other systems (at least aqua) do not like this name, so we 'catch' any errors
catch {bind $tkcanvas <KeyPress-ISO_Left_Tab> "::pd_bindings::canvas_cycle %W -1 %K %A 1 %k" } stderr

# window protocol bindings
Copy link
Contributor

Choose a reason for hiding this comment

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

why are they removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because with the "goto" functionality you can go directly to any object you want in the patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

so? with your patches you don't need the mouse any more, should we remove that as well ;-) ?
(just to make sure: the answer to this rhetoric question should be "no").

as a sidenote: if you want to remove these bindings (and i obviously don't agree that you should) for de-crufting purposes, you should also remove:

  • the canvas_cycle tcl-proc
  • the cycleselect method of the canvas
  • the canvas_cycleselect function

Copy link
Contributor

Choose a reason for hiding this comment

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

an i find goto + trying out all those indices considerably more complicated that just Tabing through the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea with "Go To" is that you don't have to try the indices. You just look at the object, see it's index, type it and hit return.

Tab-traversal is (pratically) unpredictable because the object order can change. A simple, straight"chain" of objects might have completely "spaced" indices depending on the order you created the objects, the copy-pastes you did, and etc.

Especially on big patches it can be quite a chore to keep tabbing until you get to the object you want. Even now that you can see the indices and know how many times you need to press the Tab.

What if you must press it 45 times? And holding Tab is no good either. While holding it did it triggered 30 times? 40 ? 60? I can't see how's that more complicated than

  1. press mod+g
    • all object indices are automatically visible
  2. look at the object you wanna go and type it's index
  3. Return

Also the "Go To" is originally intended for "jumps" since when you're on a "chain" you can just mod+arrow your way around with ease.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, this is mostly bike-shedding. but here you go:

What if you must press it 45 times? And holding Tab is no good either. While holding it did it triggered 30 times? 40 ? 60?

so what if? why would it matter if tab triggered 30 times, 40 or 60? i really don't care as it eventually gets me there.
and that is very simple to check with Tab-cycling. i just keep the key pressed until the object highlights. then i let go. since i was to slow, i back-cycle 3 times. done.
no symbolic processing involved (as in: relate numbers to objects).
to be fair, it might help to have a more contrasting colour that blue/black.

otoh with goto, if you have a fairly large patch, having numbers splattered all over the patch won't help readability either: so you have random numbers spilled all over the objects which makes it hard to read both the objects (beneath the numbers) and the numbers themselves. 😛

doc/7.stuff/keyboard.navigation/keyboard.patching-help.pd Outdated Show resolved Hide resolved
@HenriAugusto
Copy link
Contributor Author

Note: At the current state I've changed Find Again to Shift+accel+F and the Go to dialog is assigned to accel+G

@HenriAugusto
Copy link
Contributor Author

HenriAugusto commented May 17, 2020

Now the GUI keeps track of which patch is keyboard-navigating and we can have context-sensitive hotkeys. I've also included an entry in the Edit menu to enable/disable keyboard navigation.

Some key changes (pun intended)

  • To toggle object indices visibility: mod+i
  • Magnetic connector:
    • c+ non fan-out
    • Shift+c: fan-out

Since moving kbdnav hotkeys to TCL we shouldn't be using kn_moddown.
Removing this check allows triggering the "keyboard connect" while
kbd-navigating
Also don't mess with toplevel bindings in the "Go To" and
"Connect Object" dialogs.
@HenriAugusto
Copy link
Contributor Author

Did a cleanup and update of the first post with the current state of things.

Recently i just realized that the shift+digits are system/locale dependent. I'm inclined to change it to mod+digits.

@umlaeute umlaeute added feature suggestion for an enhancement subject:patching things concerning the patching workflow ("intelligent patching") labels Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature suggestion for an enhancement subject:patching things concerning the patching workflow ("intelligent patching")
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants