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

Set focus to filter in search #668

Merged

Conversation

jeanlaroche
Copy link
Contributor

Two tiny changes to help the workflow when selecting accounts.
Sets the focus to the filter edit cell when the find dialog is created (i shortcut). So you can start typing right away.
Expand the account tree fully when the "select account" dialog is created (so you can start typing and any subaccount can be found).

@Bob-IT
Copy link
Contributor

Bob-IT commented Mar 17, 2020

The first commit is not required, it is already fixed in maint so you can drop that.
The second one I am not sure off, I am wondering if it would be better to add a button to do the 'expand all'. Depending on there account structure, they may be just looking for 'Expenses' so they can do 'shift and +' to expand all expenses, just about choice really.

@jeanlaroche
Copy link
Contributor Author

jeanlaroche commented Mar 17, 2020

The first commit is not required, it is already fixed in maint so you can drop that.

? I rebased before doing anything, and the focus wasn't fixed. Do you mean in master perhaps?

The second one I am not sure off, I am wondering if it would be better to add a button to do the 'expand all'. Depending on there account structure, they may be just looking for 'Expenses' so they can do 'shift and +' to expand all expenses, just about choice really.

OK. Again, feel free to reject.

@Bob-IT
Copy link
Contributor

Bob-IT commented Mar 17, 2020

? I rebased before doing anything, and the focus wasn't fixed. Do you mean in master perhaps?

Just checked, second to last commit on maint, I did the change in the glade file.

@jeanlaroche
Copy link
Contributor Author

jeanlaroche commented Mar 17, 2020

? I rebased before doing anything, and the focus wasn't fixed. Do you mean in master perhaps?

Just checked, second to last commit on maint, I did the change in the glade file.

I missed that. It wasn't working for me before I made the change. I'll undo my change and retest. Thanks. P.S. A lot more elegant in the glade file. Good to know.

@jeanlaroche jeanlaroche force-pushed the set_focus_to_filter_in_search branch from 9ea1c70 to 6d118a5 Compare March 17, 2020 16:23
@jralls
Copy link
Member

jralls commented Apr 5, 2020

The catch is that while this is a productivity boost for users who want to type at the picker to get the account they want it's the opposite for users who want to use their mouse in a large tree. How about putting it in a signal handler attached to key-press-event on the treeview so that if the user starts typing ?

@jeanlaroche
Copy link
Contributor Author

The catch is that while this is a productivity boost for users who want to type at the picker to get the account they want it's the opposite for users who want to use their mouse in a large tree. How about putting it in a signal handler attached to key-press-event on the treeview so that if the user starts typing ?

Sounds great! I'll try that.

@jeanlaroche jeanlaroche force-pushed the set_focus_to_filter_in_search branch from 6d118a5 to b561dec Compare April 7, 2020 02:33
@jeanlaroche
Copy link
Contributor Author

The catch is that while this is a productivity boost for users who want to type at the picker to get the account they want it's the opposite for users who want to use their mouse in a large tree. How about putting it in a signal handler attached to key-press-event on the treeview so that if the user starts typing ?

Sounds great! I'll try that.

Wow this works really well, feels pretty natural, you can start typing part of a sub-account name and boom the subaccount opens. I'm wondering if we shouldn't allow the same thing in the main view (the account hierarchy) I think that would be super useful.

@jralls
Copy link
Member

jralls commented Apr 7, 2020

That's a bit of a different case. Ideally you'd expand the view so that the type ahead search would work (you could even reuse your type ahead code for the combo box ) but then put it back the way the user had it, save for the selected account path, once the account was selected.

@jeanlaroche
Copy link
Contributor Author

That's a bit of a different case. Ideally you'd expand the view so that the type ahead search would work (you could even reuse your type ahead code for the combo box ) but then put it back the way the user had it, save for the selected account path, once the account was selected.

True. It would be super annoying to have the current tree change from under you.
Question though: Why did the devs create the combo box with its convoluted code (auto-completion etc), instead of relying on the search provided by gtk? Just so people could see the filtered account names so they could further pick among them?

@christopherlam
Copy link
Contributor

Why did the devs create the combo box with its convoluted code (auto-completion etc), instead of relying on the search provided by gtk? Just so people could see the filtered account names so they could further pick among them?

The UI code may have predated GTK? Are you up for replacing the combo code with GtkComboBox? If you are looking for a project, there's a regression from a few years ago https://github.com/Gnucash/gnucash/blob/maint/gnucash/gnome-utils/gnc-currency-edit.c#L48 that I'd like to have fixed so that you could select a currency by typing instead of scrolling endlessly.

@jeanlaroche
Copy link
Contributor Author

jeanlaroche commented Apr 7, 2020

Why did the devs create the combo box with its convoluted code (auto-completion etc), instead of relying on the search provided by gtk? Just so people could see the filtered account names so they could further pick among them?

The UI code may have predated GTK? Are you up for replacing the combo code with GtkComboBox? If you are looking for a project, there's a regression from a few years ago https://github.com/Gnucash/gnucash/blob/maint/gnucash/gnome-utils/gnc-currency-edit.c#L48 that I'd like to have fixed so that you could select a currency by typing instead of scrolling endlessly.

Wait, I can start typing the currency name and I get an autocomplete (in the account editing dialog). Are you saying that's not working as expected?

@christopherlam
Copy link
Contributor

Wait, I can start typing the currency name and I get an autocomplete (in the account editing dialog). Are you saying that's not working as expected?

Doesn't work in report options eg Transaction Report, Common Currency. Anyway this is not germane to this PR, so feel free to ignore me butting in...

@Bob-IT
Copy link
Contributor

Bob-IT commented Apr 7, 2020

OK, gave it a spin and works but one suggestion which can be ignored.
Can the key press be limited to alpha numeric keys and with no key combinations.
If I have a collapsed tree and highlight say, the assets account, I would still like to press + to expand just that level or 'shift and +' to expand all at that level.

@jeanlaroche
Copy link
Contributor Author

OK, gave it a spin and works but one suggestion which can be ignored.
Can the key press be limited to alpha numeric keys and with no key combinations.
If I have a collapsed tree and highlight say, the assets account, I would still like to press + to expand just that level or 'shift and +' to expand all at that level.

That makes complete sense, we don't want to disable the existing shortcuts.

@jralls
Copy link
Member

jralls commented Apr 7, 2020

Only the register predates Gtk, but GnuCash was one of the early adopters of Gtk and much of the GUI code goes back to around the time that Gtk2 was first released because of a large conversion effort at that time. Since then, and especially for the migration to Gtk3 that was done in a hurry, updates have been limited to keeping things working.

I think it would be better to filter specific shortcuts for GtkTreeView than to try figure out what keypresses represent text. The latter depends too much on keyboard layouts for different operating systems and locales, never mind input methods for writing systems that don't map well to keyboards.

@jeanlaroche
Copy link
Contributor Author

I think it would be better to filter specific shortcuts for GtkTreeView than to try figure out what keypresses represent text. The latter depends too much on keyboard layouts for different operating systems and locales, never mind input methods for writing systems that don't map well to keyboards.

Yes, if the keypress event only has a key value (not mapped to a specific character) this would be a pain. I'll take a look.

@jeanlaroche
Copy link
Contributor Author

OK, gave it a spin and works but one suggestion which can be ignored.
Can the key press be limited to alpha numeric keys and with no key combinations.
If I have a collapsed tree and highlight say, the assets account, I would still like to press + to expand just that level or 'shift and +' to expand all at that level.

I implemented your suggestion.

@Bob-IT
Copy link
Contributor

Bob-IT commented Apr 8, 2020

@jeanlaroche
I think I would of gone with something like this, what do you think..

if (event->length == 0)
    return FALSE;

switch (event->keyval)
{
    case GDK_KEY_plus:
    case GDK_KEY_minus:
    case GDK_KEY_asterisk:
    case GDK_KEY_slash:
    case GDK_KEY_KP_Add:
    case GDK_KEY_KP_Subtract:
    case GDK_KEY_KP_Multiply:
    case GDK_KEY_KP_Divide:
        return FALSE;
    break;
    default:
        gtk_tree_view_expand_all (GTK_TREE_VIEW(user_data));
    return FALSE;
}

@jeanlaroche
Copy link
Contributor Author

@jeanlaroche
I think I would of gone with something like this, what do you think..

if (event->length == 0)
    return FALSE;

switch (event->keyval)
{
    case GDK_KEY_plus:
    case GDK_KEY_minus:
    case GDK_KEY_asterisk:
    case GDK_KEY_slash:
    case GDK_KEY_KP_Add:
    case GDK_KEY_KP_Subtract:
    case GDK_KEY_KP_Multiply:
    case GDK_KEY_KP_Divide:
        return FALSE;
    break;
    default:
        gtk_tree_view_expand_all (GTK_TREE_VIEW(user_data));
    return FALSE;
}

What do * and / do ? Are they shortcuts in the tree view? But your switch() statement does work for sure. Using the string is a shortcut to get the same result for GDK_KEY_plus and GDK_KEY_KP_Add (for example).

@Bob-IT
Copy link
Contributor

Bob-IT commented Apr 8, 2020

What do * and / do

They are equivalent to shifting + and -

  • will expand that level, shift and + or * will expand all child levels
  • / will collapse that level

@jeanlaroche
Copy link
Contributor Author

Ah ok, I missed that! Then they definitely have to be added! I could add them in a silly manner with "*" and "/" or using your switch() statement.
Anybody else has an opinion on which way is going to be more desirable/robust?

@jralls
Copy link
Member

jralls commented Apr 8, 2020

There are a ton of keybindings, see gtktreeview.c starting at line 1600.

@jralls
Copy link
Member

jralls commented Apr 8, 2020

Since the keybindings are to specific keysyms rather than the character they emit using the keysyms will be more robust.

@jeanlaroche
Copy link
Contributor Author

OK then I'll use Bot's switch statement and add all the key bindings. Thanks for pointing them out.

@jralls
Copy link
Member

jralls commented Apr 13, 2020

Looks good to me. @Bob-IT ?

@Bob-IT
Copy link
Contributor

Bob-IT commented Apr 14, 2020

OK with me, @jeanlaroche can you squash this to one commit please.

@jeanlaroche jeanlaroche force-pushed the set_focus_to_filter_in_search branch from e7c9830 to 524c90e Compare April 14, 2020 15:06
@code-gnucash-org code-gnucash-org merged commit ee57e0f into Gnucash:maint Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants