Fix header content size when sorting is disabled
ClosedPublic

Authored by astan on Feb 2 2017, 2:40 PM.

Details

Summary

Instead of always adding space for the sorting indicator in item view headers, only add it if sorting is enabled. Without this fix, operations such as QTreeView::resizeColumnToContents(..) will not result in a snug fit when the header section is wider than all items in the column.

Test Plan
#include <QApplication>                                                                                                               
#include <QTreeView>
#include <QStandardItemModel>
#include <QStandardItem>

int main(int argc, char *argv[])
{
    QApplication app(argc, argv);

    // Test model
    QStandardItemModel model(3, 2);
    model.setHorizontalHeaderLabels({ "Header 1", "Header 2" });
    for (int row = 0; row < 3; ++row) {
        for (int column = 0; column < 2; ++column) {
            model.setItem(row, column, new QStandardItem("Foo"));
        }
    }

    // View with sorting disabled
    QTreeView view;
    view.setWindowTitle("Sorting Disabled");
    view.setModel(&model);
    view.show();
    view.resizeColumnToContents(0);

    // View with sorting enabled
    QTreeView viewWithSorting;
    viewWithSorting.setWindowTitle("Sorting Enabled");
    viewWithSorting.setModel(&model);
    viewWithSorting.setSortingEnabled(true);
    viewWithSorting.show();
    viewWithSorting.resizeColumnToContents(0);

    return app.exec();
}

Notice how before applying this fix, there's space reserved for the sorting indicator even on the QTreeView that has sorting disabled.

Before the fix:

After the fix:

Diff Detail

Repository
R31 Breeze
Branch
fix-unsortable-item-view-header-size (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
astan updated this revision to Diff 10850.Feb 2 2017, 2:40 PM
astan retitled this revision from to Fix header content size when sorting is disabled.
astan updated this object.
astan edited the test plan for this revision. (Show Details)
astan added a reviewer: Breeze.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 2 2017, 2:40 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
astan updated this object.Feb 2 2017, 2:42 PM
astan edited the test plan for this revision. (Show Details)
astan edited the test plan for this revision. (Show Details)
cfeck added a subscriber: cfeck.Feb 2 2017, 2:47 PM

QCommonStyle also adds the margins only when a sort indicator is needed, so +1

astan added a comment.Feb 2 2017, 3:00 PM

@cfeck: Right, and so does Fusion. After a long hiatus from KDE, this is the first time I use Phabricator (really like it!): Is a +1 enough to push, or is there a more formal approval mechanism?

I have added Hugo (Breeze and Oxygen style maintainer).

hpereiradacosta accepted this revision.Feb 2 2017, 3:55 PM
hpereiradacosta added a reviewer: hpereiradacosta.

Thanks for the patch ! Ship it !
Also, it would be great if you could see if it also applies to oxygen

This revision is now accepted and ready to land.Feb 2 2017, 3:55 PM
astan closed this revision.Feb 2 2017, 4:54 PM
astan added a comment.Feb 2 2017, 5:10 PM

@hpereiradacosta: The problem and fix was the same in Oxygen. I opened https://phabricator.kde.org/D4410 for that.