Sort grid items properly

[Chocobo1]

• GHA CI: add checks for grid items order
  Now all items under QGridLayout are required to be sorted. This allow
  us to omit tabstop order. The tabstop order will follow the layout order.
  The script can be invoked to fix wrong grid items order in .ui files:

  python check_grid_items_order.py file.ui

• Sort grid items properly
  Supersedes Fixed tab stop order in the Options Dialog, Connection page #21856.
• Make links accessible by keyboard
• Make tab key switch focus
  These fields do not expect tab characters.

[glassez]

• Now all items under QGridLayout are required to be sorted. This allow
  us to omit tabstop order. The tabstop order will follow the layout order.
  The script can be invoked to fix wrong grid items order in .ui files:

  python check_grid_items_order.py file.ui
  

I believe it should be stated in "Coding Guidelines".

[thalieht]

RSS downloader:

1. Tabs go Import>Export>Import again.
2. Tab that's supposed to enter Torrent parameters section doesn't, Also the tab order in some elements in there is wrong.

RSS tab (in main window):
The 3 vertical sections, don't know their names, the tab order goes from right to left instead of the other way around.
Also isn't RSS feeds (header?) that sorts the feeds supposed to be tabbable? If it's a header i guess not.

Torrent creator:
Tabs go Create Torrent>Cancel>Create Torrent again.

Some discrepancies that may be off topic (i didn't test the behavior before this PR):
About dialog:
The links in About and Authors are not tabbable unlike the links in the other tabs.
In Software used i don't know what is selected with the tab after Copy to clipboard button.

Options dialog:
No link is tabbable. I wouldn't mention it but some links are tabbable in the dialog mentioned above.

Add torrent dialog:
I don't know what is selected with the next 2 tabs after the Content layout combobox.
Also the tab after Filter files...
And something that breaks the tab flow:
Some text edits with the acceptRichText property don't have the tabChangesFocus property.

And finally, something definitely off topic:
The labels in Advanced shouldn't be tabbable.

[Chocobo1]

Thanks for the intensive testing!

RSS downloader:
Tabs go Import>Export>Import again.

It actually doesn't go back to Import at the last step. It goes to somewhere else which I couldn't find out... The Import button is highlighted but it doesn't actually have focus.
(This behavior is present in master, i.e. not a regression of this PR)

Tab that's supposed to enter Torrent parameters section doesn't, Also the tab order in some elements in there is wrong.

The "Torrent parameters" have layout coded in c++ and Qt couldn't figure out the correct tab order. It doesn't seem feasible to fix it in this PR. Let's defer this.
(This behavior is present in master, i.e. not a regression of this PR)

RSS tab (in main window):
The 3 vertical sections, don't know their names, the tab order goes from right to left instead of the other way around.

Will submit another PR after this PR is merged. See PR #21999.

Also isn't RSS feeds (header?) that sorts the feeds supposed to be tabbable? If it's a header i guess not.

It is a header.

Torrent creator:
Tabs go Create Torrent>Cancel>Create Torrent again.

Same situation as the RSS "Import>Export>Import".

Some discrepancies that may be off topic (i didn't test the behavior before this PR):
About dialog:
The links in About and Authors are not tabbable unlike the links in the other tabs.

Added a commit to fix it: Make links accessible by keyboard

In Software used i don't know what is selected with the tab after Copy to clipboard button.

Couldn't find out either...

Options dialog:
No link is tabbable. I wouldn't mention it but some links are tabbable in the dialog mentioned above.

Added a commit to fix it: Make links accessible by keyboard

Add torrent dialog:
I don't know what is selected with the next 2 tabs after the Content layout combobox.

One might be the Comment field but I couldn't find out the other one...

Also the tab after Filter files...

It goes to the outer vertical scroll bar.

And something that breaks the tab flow:
Some text edits with the acceptRichText property don't also have the tabChangesFocus property.

Nice find!
Added another commit for it: Make tab key switch focus

And finally, something definitely off topic:
The labels in Advanced shouldn't be tabbable.

Looked into it but didn't found a solution...

[Chocobo1]

• Now all items under QGridLayout are required to be sorted. This allow
  us to omit tabstop order. The tabstop order will follow the layout order.
  The script can be invoked to fix wrong grid items order in .ui files:

  python check_grid_items_order.py file.ui
  

I believe it should be stated in "Coding Guidelines".

Maybe later.

The "CI - File health" will actually fail if it is violated so we can rely on it to enforce this rule. And it will emit a diff to show how to fix it.

[glassez]

• Now all items under QGridLayout are required to be sorted.

Note that this requirement will break experience of WYSIWYG widgets editing. Are there valid reasons to do this?

[Chocobo1]

Note that this requirement will break experience of WYSIWYG widgets editing.

You can still use Qt designer tools to edit but you still need to make sure the order is sorted.

Are there valid reasons to do this?

#21856 (comment)

[thalieht]

Added another commit for it: Make tab key switch focus

Some more fields in options (their names in code):
textExcludedFileNames textTrackers textSmartEpisodeFilters textWebUICustomHTTPHeaders

Added a commit to fix it: Make links accessible by keyboard

You forgot List of alternative WebUI link.
Also i have something strange to report:
I can't shift+tab to go to the previous element from Information about certificates and Reverse proxy setup examples but i can from the anonymous mode link.

/off topic
I just noticed labelReverseProxyLink defines a color (#0000ff) which is almost unreadable in dark theme.

[Chocobo1]

Added another commit for it: Make tab key switch focus

Some more fields in options (their names in code): textExcludedFileNames textTrackers textSmartEpisodeFilters textWebUICustomHTTPHeaders

Fixed.

Added a commit to fix it: Make links accessible by keyboard

You forgot List of alternative WebUI link.

Fixed.

Also i have something strange to report: I can't shift+tab to go to the previous element from Information about certificates and Reverse proxy setup examples but i can from the anonymous mode link.

I noticed it too. Don't know why.

/off topic I just noticed labelReverseProxyLink defines a color (#0000ff) which is almost unreadable in dark theme.

Revised. It is still the same for me in Qt designer Preview. Does it work for you?

[thalieht]

/off topic I just noticed labelReverseProxyLink defines a color (#0000ff) which is almost unreadable in dark theme.

Revised. It is still the same for me in Qt designer Preview. Does it work for you?

Yes, now it's like the other links.

[glassez]

Note that this requirement will break experience of WYSIWYG widgets editing.

You can still use Qt designer tools to edit but you still need to make sure the order is sorted.

Are there valid reasons to do this?

#21856 (comment)

UI files do not seem to be intended for "manual" use (under regular workflow). It is just an internal format of Qt Designer. IMO, the only problem with this is that design changes made using Qt Designer often produce "ugly" diffs. So I myself preferred to improve them manually after that, and (obviously) such a workflow is not a problem for me.
The grid items Qt Designer placed randomly also looked ugly to me, which also often became the subject of my manual editing.
But what worries me is, should we make this a requirement? Won't this be a problem for some contributors?
As I see it, you are providing python script to automatically correct the order of the grid items. But what will it look like from the point of view of convenience within the entire scenario of changing .ui files? Will the contributor be easily notified of how he could use this script to fix the problem?
(And I always catch myself thinking in such cases, could such corrections be made automatically?)

[Chocobo1]

Will the contributor be easily notified of how he could use this script to fix the problem?

It is of course not immediately obvious to the contributor that there exists such script to help them. However when this specific check failed, the error message will include a diff which should help contributors fixing the code. Or we can just remind the contributor to use the python script.
IMO it shouldn't be too much of a problem in reality since .ui files are updated rarely. And IIRC most updates are done by frequent contributors instead of outside contributors.

But what worries me is, should we make this a requirement? Won't this be a problem for some contributors?

IMO it is not too different than other CI checks. Once a check fails, the author should just fix it.

(And I always catch myself thinking in such cases, could such corrections be made automatically?)

It is possible to automate these checks. One can setup pre-commit on their local machine. This is the framework "CI - File health" is using.
Or github actions can push additional 'fix' commit on the branch. But I don't really prefer the latter approach. I suppose pushing commits without notifying the PR author could be rude and might cause more problems if the author isn't good at git/github collaboration.

[glassez]

However when this specific check failed, the error message will include a diff which should help contributors fixing the code. Or we can just remind the contributor to use the python script.

I believe such a script should be mentioned in error message (if possible). Seeing just the diff doesn't look friendly to someone who just edited some widget in the WYSIWYG editor.

And IIRC most updates are done by frequent contributors instead of outside contributors.

This is the current state of affairs. But I wouldn't consider it a solid prerequisite.

It is possible to automate these checks. One can setup pre-commit on their local machine. This is the framework "CI - File health" is using.
Or github actions can push additional 'fix' commit on the branch. But I don't really prefer the latter approach. I suppose pushing commits without notifying the PR author could be rude and might cause more problems if the author isn't good at git/github collaboration.

Okay, I agree.
Perhaps someone will provide further on qBittorrent Wiki detailed instructions on how to set up such pre-commits locally to perform all our "file health" related fixes.

[Chocobo1]

I believe such a script should be mentioned in error message (if possible).

PR updated.
I added a message in the script that will hint such script is available to fix the defect. This should be sufficient for the contributors whom read the CI error logs.

[xavier2k6]

@Chocobo1 Bump "pre-commit-hook" rev to v5.0.0 (has a new hook available, check-illegal-windows-names) & "typos" to 1.28.3 here? or to be done in a separate PR??

[Chocobo1]

@Chocobo1 Bump "pre-commit-hook" rev to v5.0.0 (has a new hook available, check-illegal-windows-names) & "typos" to 1.28.3 here? or to be done in a separate PR??

A separate PR.