Differential D4287 Diff 10731 plugins/filetemplates/templateclassassistant.cpp

Changeset View

Standalone View

View Options

plugins/filetemplates/templateclassassistant.cpp

Show First 20 Lines • Show All 498 Lines • ▼ Show 20 Line(s)420{
499 } 499​ }
500 else if (d->templateOptionsPage && (currentPage() == d->templateOptionsPage)) 500​ else if (d->templateOptionsPage && (currentPage() == d->templateOptionsPage))
501 { 501​ {
502 d->templateOptionsPageWidget->load(d->fileTemplate, d->renderer); 502​ d->templateOptionsPageWidget->load(d->fileTemplate, d->renderer);
503 } 503​ }
504 else if (currentPage() == d->outputPage) 504​ else if (currentPage() == d->outputPage)
505 { 505​ {
506 d->outputPageWidget->loadFileTemplate(d->fileTemplate, d->baseUrl, d->renderer); 506​ d->outputPageWidget->loadFileTemplate(d->fileTemplate, d->baseUrl, d->renderer);
507 } 507​ }
arrowdUnsubmitted
Not Done

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

arrowd: Can't we just do `currentPage()->setFocusToFirstEditWidget()` after all those `if`s?
kossebauAuthorUnsubmitted
Not Done

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?
kossebau: Good point, I forgot to ask some questions I have about this: # The repeated explicit calls…
mwolffUnsubmitted
Not Done
  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?

mwolff: 1. The pages are under our control, right? so it's not an optional feature? So adding an…
kossebauAuthorUnsubmitted
Not Done

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.

kossebau: Re 1. Updated patch for a version with a custom interface. Might later perhaps make this a…
508​
509​ if (auto* pageFocus = dynamic_cast<KDevelop::IPageFocus*>(currentPage()->widget())) {
510​ pageFocus->setFocusToFirstEditWidget();
511​ }
508} 512​}
509 513​
510void TemplateClassAssistant::back() 514​void TemplateClassAssistant::back()
511{ 515​{
512 KAssistantDialog::back(); 516​ KAssistantDialog::back();
513 if (currentPage() == d->templateSelectionPage) 517​ if (currentPage() == d->templateSelectionPage)
514 { 518​ {
515 REMOVE_PAGE(classIdentifier) 519​ REMOVE_PAGE(classIdentifier)
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines