Always highlight the OK button when installing an add-on

[gusandrianos]

Description

This solves #16754.
Admittedly this is somewhat of a personal opinion issue, although I think it is a valid one. It can probably be a better fix by highlighting the "cancel" button first, in order to prevent accidental clicks to the "OK" button.

Motivation and Context

How Has This Been Tested?

I tested on Linux Mint 19.2

Screenshots (if appropriate):

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[a1rwulf]

Please set Milestone and Labels ;-)
I think it's perfectly fine to set the default selection to OK when you want to install something.
We discussed this at devcon as well.

[gusandrianos]

@a1rwulf I can't recall we did, I will.

[a1rwulf]

Probably you weren't part of the discussion, but @kib and some others talked about the exact same thing.
Highlighting OK" is ok when you ask for confirmation of an action like installing. If you want to prevent the user from a mistake, Cancel` should be the default.
Eg.:
Do you really want to delete this file?

[Hitcher]

Just to be certain - this only applies when installing addons right?

[gusandrianos]

@HitcherUK AFAIK-tested, yes.

[ronie]

i'm afraid this isn't correct.
afaik the code you've modified is used to set focus to the 'detailed list' when needed.

skins hardcode the focus to the simple list. kodi forces focus to the detailed list in case the detailed list is visible.

you check check it is broken now by going to Settings > Interface > Skin.
it should focus the list of skins, but with this pr the 'get more' button is focused.

[da-anda]

Probably you weren't part of the discussion, but @kib and some others talked about the exact same thing.
Highlighting OK" is ok when you ask for confirmation of an action like installing. If you want to prevent the user from a mistake, Cancel` should be the default.
Eg.:
Do you really want to delete this file?

correct, that's what we decided on during devcon

[garbear]

My comment here might be of some use

[gusandrianos]

Jenkins build this please

[DaveTBlake]

@ronie are you happy with this now?

[ronie]

@DaveTBlake yup, works ok now.

[Montellese]

This breaks the possibility to focus a dependency in the list of dependencies and view its details.

[Montellese]

c3bc22d should fix the issue.

[gusandrianos]

@Montellese This was the intention. If you want to view the details in one of the items, you move to it and you do so. More often than not, you just install the add-on and it makes sense to have "OK" highlighted. This was discussed during this year's devcon and it was a team decision.

[ronie]

you move to it and you do so

please try that ;-)

i can confirm the issue @Montellese reported.

[Montellese]

I'm perfectly fine with having the OK button pre-select but now it is impossible to move the focus to one of the dependencies in the list because your changes to CGUIDialogSelect::OnMessage() prevent the possibility to focus the list at all if m_focusToButton has been set. I'm pretty sure that was not the intention.

With my change the OK button is still pre-focused and you can also change the focus to all the other controls in the dialog (including the list of dependencies).

[gusandrianos]

I'll take a look tomorrow, thanks for the heads up, I don't think I noticed it but maybe I didn't look for it to do so (?)

I'm having a bit of a transition period due to getting a new job, if @ronie approves go ahead and merge it otherwise I'll review it tomorrow night.

[Montellese]

See #17115 for a PR containing the commit mentioned above.

[Montellese]

@gusandrianos I also only found out by accident that I can select one of the dependencies and navigate to its details.