File templates assistant: focus first edit field on switch to new page
ClosedPublic

Authored by kossebau on Jan 26 2017, 12:42 AM.

Details

Summary

Most pages have fields which are expected to be edited, so setting the
keyboard focus to the first item on the page saves the user some work
on average. With the Enter key being bound to the "Next" button, this
should also not add work to pages where default settings are okay.

Diff Detail

Repository
R33 KDevPlatform
Branch
fixFileTemplatesDialog
Lint
No Linters Available
Unit
No Unit Test Coverage
kossebau updated this revision to Diff 10570.Jan 26 2017, 12:42 AM
kossebau retitled this revision from to File templates assistant: focus first edit field on switch to new page.
kossebau updated this object.
kossebau added a reviewer: KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJan 26 2017, 12:42 AM
arrowd added a subscriber: arrowd.Jan 26 2017, 8:14 AM
arrowd added inline comments.
plugins/filetemplates/templateclassassistant.cpp
507

Can't we just do currentPage()->setFocusToFirstEditWidget() after all those ifs?

kossebau added inline comments.Jan 26 2017, 3:16 PM
plugins/filetemplates/templateclassassistant.cpp
507

Good point, I forgot to ask some questions I have about this:

  1. The repeated explicit calls could be avoided by making setFocusToFirstEditWidget() a part of the interface. What would be the best (KDevelop) style here: a) add an abstract interface which all pages would optionally implement and which would be queried here first or b) add another subclass with that abstract method added which all pages need to subclass from?
  2. Would it make more sense to set the first edit widget as focus proxy to the page container? So there is no need for a special call with a given semantic setFocusToFirstEditWidget() here we just need to call setFocus() on the page? That approach would also mean any click in the page background would set the keyboard focus to the first edit widget, is that expected?
mwolff added a subscriber: mwolff.Jan 29 2017, 8:10 PM
mwolff added inline comments.
plugins/filetemplates/templateclassassistant.cpp
507
  1. The pages are under our control, right? so it's not an optional feature? So adding an interface for that would make sense to me.
  1. I have no clue about focus proxies

but, in general, isn't there a way to set the widget focus from designer and couldn't that be used instead of all this manual code?

kossebau updated this revision to Diff 10686.Jan 29 2017, 9:52 PM

switch to use a new IPageFocus interface

kossebau added inline comments.Jan 29 2017, 10:01 PM
plugins/filetemplates/templateclassassistant.cpp
507

Re 1. Updated patch for a version with a custom interface. Might later perhaps make this a global kdevplatform interface and use it in other assistants as well, but for now kept the interface class in the plugin.

Re in general. Not that I am aware of. At least not when it comes to focus setting on switching between widgets, and more, if for switching between pages other widgets are used, here the buttons, which also can have keyboard focus.

KPageDialog/KAssistantDialog itself does not do any focus handling, leaves that all to implementors, for good and for bad.

mwolff accepted this revision.Jan 30 2017, 9:50 AM
mwolff added a reviewer: mwolff.

minor nitpicks, feel free to commit noce they are resolved.

and note: you are going overkill here, no need for the Qt Interface system, a simply abstract base class is enough. The template system does not support external plugins that could provide the pages after all (or am I missing something)? And we support RTTI, so you could use dynamic_cast and be done with it. But if you prefer, you can keep it as-is.

plugins/filetemplates/classmemberspage.cpp
31

this is c++17, no? please don't use that yet

plugins/filetemplates/ipagefocus.cpp
25

= default;

This revision is now accepted and ready to land.Jan 30 2017, 9:50 AM
kossebau added inline comments.Jan 30 2017, 2:39 PM
plugins/filetemplates/classmemberspage.cpp
31

If it is, there are 10 other places in existing KDevPlatform code to fix :) Unless I missed a detail?

plugins/filetemplates/ipagefocus.cpp
25

Need hand-helping, not yet into C++11, how should the code look exactly?

This revision was automatically updated to reflect the committed changes.
kossebau marked an inline comment as done.