Packages Panel in QuickStart Wizard#2845
Packages Panel in QuickStart Wizard#2845sunderme merged 2 commits intotexstudio-org:masterfrom octaeder:quickDoc
Conversation
src/quickdocumentdialog.cpp
Outdated
| QTableWidget *table = ui.tableWidgetPackages; | ||
| table->clear(); | ||
| table->setColumnCount(2); | ||
| table->setHorizontalHeaderLabels( QStringList( { tr("Package"), tr("Description") } ) ); |
There was a problem hiding this comment.
isn't this already defined in the .ui file ?
There was a problem hiding this comment.
no
<item row="1" column="0">
<widget class="QTableWidget" name="tableWidgetPackages">
<property name="sizePolicy">
<sizepolicy hsizetype="Expanding" vsizetype="Expanding">
</sizepolicy>
</property>
</widget>
</item>
There was a problem hiding this comment.
arraydialog.ui hasn't it (l. 35, obviously because we don't no the columns), bibtexdialog.ui has it (l. 55). So I'll move it, even so I think that it makes translations more complicate.
There was a problem hiding this comment.
I was mislead. This doesn't work, reason is calling tableCear() in the init function which is (indirectly) called in each addXxx function. So I returned to my solution.
There was a problem hiding this comment.
Ah, good hint, that helps. Ok, prepared.
src/quickdocumentdialog.cpp
Outdated
|
|
||
| //each QStringList holds 2 items: the name of the package, and a short package description. These constitute a row of the table of the packages tab | ||
| QList<QStringList> packages = QList<QStringList>() | ||
| << QStringList( {"amssymb" , "Mathematical symbols from AMS"} ) |
There was a problem hiding this comment.
The description needs to be translatable.
src/quickdocumentdialog.cpp
Outdated
| itemPkgName->setFlags(Qt::ItemIsEnabled | Qt::ItemIsSelectable | Qt::ItemIsUserCheckable); | ||
| itemPkgDescription->setFlags(Qt::ItemIsEnabled | Qt::ItemIsSelectable); | ||
|
|
||
| table->setRowCount(row+1); |
There was a problem hiding this comment.
probably not the most efficient way to build a table, but in this context it does not matter.
There was a problem hiding this comment.
What's the point? Always setting RowCount?
There was a problem hiding this comment.
yes, especially as you know the number from the start.
There was a problem hiding this comment.
The reason for that was that the wizard crashed unexpectedly and it took me hours to figure out that this is missing. This happend because I coded the connects where all other connects are placed. But when setting the checkState for each row inserted in the table a signal was generated to set the row selected, which triggered signals to set checkStates appropriate. But checkStates has to run over the whole table, but not all rows had been filled. When I understood this I moved the connects for the signals after the table is setup. So it should now be possible to set it after reading the packages list. I will retest this.
There was a problem hiding this comment.
Works as expected with table->setRowCount(packages.size()); before the loop.
| itemPkgName->setCheckState( found ? Qt::Checked : Qt::Unchecked ); | ||
| updateSelected(row); | ||
| } | ||
| int height = ui.GridPackages->geometry().height(); |
There was a problem hiding this comment.
this looks odd.
Why is it necessary ?
There was a problem hiding this comment.
You mean referencing updateSelected? This ensures that the line having checkState set appears selected. Otherwise one could click in a row (say in second column) which turns the row selected (blue) but the checkbox isn't selected or in a checkbox but row is not selected. Thus this is done to avoid visual ambiguity.
There was a problem hiding this comment.
also the height stuff.
There should be automatisms for that.
There was a problem hiding this comment.
Maybe, I used hours to figure out which way could work and to try them. But nothing helped. I think the main reason for this is your which not to see a scrollbar at the beginning. If I don't specify the height, then Qt will distribute space between table and what is below the table. Now one could think change ratio but if the table widget height then is larger as needed an empty space appears after the rows but still inside the frame of the table widget. I have seen this when setting the spacer size policy to minimum. And as a side effect when clicking in this extra space all rows get unselected.
There was a problem hiding this comment.
Have you tried a vertical spacer below the table ?
There was a problem hiding this comment.
If you don't have a spacer then the table widget fits exactly the header plus rows, but it's centered vertically in the tab, which looks random. So we need a spacer (I already mentioned above) below the table widget that moves the table upwards. Since there is no spacer that expands an amount needed such that the table widget fits exactly the header and rows, the spacer needs a policy of minimumExpanding. But this means no other than that the spacer and the table take their space in a specific ratio, normally 1:1 (to prevent this I set the minimum and maximum height of the table to the same value). But this will introduce a scrollbar because the table needs more space. Of course we could now try to approximate the real number needed as ratio by rational numbers (you need to use integers for specifying proportions). But after adding one more row (or just changing to windows style, s. below) this will break again. I don't see how we can break the circle.
This shows what happens when switching style from fusion to windows just because the decoration needs some extra pixels (we can still see all rows):
Maybe you've overlooked the 2 in the starting height:
int tableHeight = table->horizontalHeader()->height() + 2;
This "magic number" 2 is needed to fit with fusion. With windows one would need 4 or 5 as magic number, I tried but I don't remember the exact value. Maybe you know the book saying "Don't use magic numbers in your code". But we have to.
| void QuickDocumentDialog::addUserPackages() | ||
| { |
There was a problem hiding this comment.
this dialog is a bit underwhelming, especially as txs has a list of all available packages.
But maybe a task for a future PR.
src/quickdocumentdialog.cpp
Outdated
| } | ||
| } | ||
| //when package selections change | ||
| void QuickDocumentDialog::updateCheckState() |
There was a problem hiding this comment.
I am not sure what exactly the logic is supposed to do, but basically if you select a row by clicking on a description text, all other selections/checked states are removed.
Let's stick with Qt logic, i.e. you click on the box to select something to be inserted ...
There was a problem hiding this comment.
You may try what happens if you remove my connected signals (I started development without these). But I found that checking more than three items is cumbersome since the checkboxes have a small size and it's slow (remember the project offering all known packages). Selecting by dragging the pointer over the rows or clicking into the (large) rows is quick and easy. Since this is a table (not a list) you have to use modifiers as it is known from windows file explorer or calc/excel. Thus clicking one row results in a selection containing only this row. Right, if you slightly miss a checkbox you want to click on then this row will be the only one selected. But you can redo your selection quickly and take care the next time. Maybe we could add the tooltip text before the table so help is immediately visible (s. image)?
If the wizard starts with initial selections (maybe a longer table with a scrollbar) the selected lines make it easier to recognize which rows are selected than the checkboxes on their own would do. And you can go easily through the descriptions of the selected packages without moving your eyes from left to right.
I also investigated the case when I keep the selection theme but remove the checkboxes at all. But at the first clicks I was not convinced that this is enough feedback for everyone to know that one already selected packages. So I added the checkboxes again which give a pretty clear feedback. But I don't use them for selecting for the reasons said, but you can if you want to.
This image may clarify why I wanted to synchronize row selections and checkStates:
Compare to this (I just expanded the previous selection, no signals):
So I would recommend allowing selections.
There was a problem hiding this comment.
if you need a help text to describe the task of simply selecting n of 8 packages, you are doing something wrong.
Basically you are mixing two, contradicting, use concepts into this.
We stick to Qt default (checkboxes only, no selection, nothing fancy), especially as it is not a concept used anywhere else in txs.
This is a roadblock for merging.
|
thanks |




This PR closes #2831. This mainly adds a new tab to the QuickStart Wizard named Packages, It shows in a table a small selection of packages each with a short description. Also those packages which are currently selectable with a checkbox are removed from the first tab (Class Options), as well as the combobox for the Input Encoding which is now set to utf8 in any case.