Skip to content

Comments

Packages Panel in QuickStart Wizard#2845

Merged
sunderme merged 2 commits intotexstudio-org:masterfrom
octaeder:quickDoc
Jan 13, 2023
Merged

Packages Panel in QuickStart Wizard#2845
sunderme merged 2 commits intotexstudio-org:masterfrom
octaeder:quickDoc

Conversation

@octaeder
Copy link
Contributor

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.

QTableWidget *table = ui.tableWidgetPackages;
table->clear();
table->setColumnCount(2);
table->setHorizontalHeaderLabels( QStringList( { tr("Package"), tr("Description") } ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this already defined in the .ui file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

<item row="1" column="0">
 <widget class="QTableWidget" name="tableWidgetPackages">
  <property name="sizePolicy">
   <sizepolicy hsizetype="Expanding" vsizetype="Expanding">
   </sizepolicy>
  </property>
 </widget>
</item>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be set there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, is prepared

Copy link
Contributor Author

@octaeder octaeder Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...->clearContent()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good hint, that helps. Ok, prepared.


//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"} )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description needs to be translatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, is prepared

itemPkgName->setFlags(Qt::ItemIsEnabled | Qt::ItemIsSelectable | Qt::ItemIsUserCheckable);
itemPkgDescription->setFlags(Qt::ItemIsEnabled | Qt::ItemIsSelectable);

table->setRowCount(row+1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not the most efficient way to build a table, but in this context it does not matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point? Always setting RowCount?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, especially as you know the number from the start.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@octaeder octaeder Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks odd.
Why is it necessary ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the height stuff.
There should be automatisms for that.

Copy link
Contributor Author

@octaeder octaeder Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried a vertical spacer below the table ?

Copy link
Contributor Author

@octaeder octaeder Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

grafik

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.

Comment on lines +556 to 557
void QuickDocumentDialog::addUserPackages()
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this dialog is a bit underwhelming, especially as txs has a list of all available packages.
But maybe a task for a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know

}
}
//when package selections change
void QuickDocumentDialog::updateCheckState()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

grafik

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:

grafik

Compare to this (I just expanded the previous selection, no signals):

grafik

So I would recommend allowing selections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sunderme sunderme merged commit 8a1e618 into texstudio-org:master Jan 13, 2023
@sunderme
Copy link
Member

thanks

@octaeder octaeder deleted the quickDoc branch January 13, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: A Package Panel (Tab) for the QuickStart Wizard

2 participants