Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Add support for Splitter #14

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add support for Splitter #14

wants to merge 5 commits into from

Conversation

Znurre
Copy link
Contributor

@Znurre Znurre commented Feb 19, 2018

Add support for Splitter. I am not entirely sure about putting stretch as an attached property of Splitter, since it really modifies the SizePolicy of the widget, but it makes for a nice and consistent (with HBoxLayout) API.


This file is part of DeclarativeWidgets, library and tools for creating QtWidget UIs with QML.

Copyright (C) 2013-2017 Klarälvdalens Datakonsult AB, a KDAB Group company, [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix the dates in the copyright notices please?

@0x6e
Copy link
Contributor

0x6e commented Mar 26, 2018

In principle I don't see a problem with having a stretch attached property as the original API is only a convenience on changing the size policy (something you couldn't do in DeclarativeWidgets when this patch was created).

This change looks like it would be a good candidate for an addition to the layouts unit test. Whilst not strictly a layout, there are already functions there to compare the geometry of widgets created with imperative Qt Widget and DeclarativeWidgets. Could you add a test case please, comparing layouts created using splitters please?

@Znurre
Copy link
Contributor Author

Znurre commented Mar 26, 2018

Absolutely, thanks for the feedback. I will look into this later this week.

Copy link
Contributor

@krake-kdab krake-kdab left a comment

Choose a reason for hiding this comment

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

Isn't that now already covered by the new support for size policy?


void SplitterWidgetContainer::addWidget(QWidget *widget)
{
QObject *attachedProperties = qmlAttachedPropertiesObject<QSplitter>(widget, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

that should probably be DeclarativeSplitter as the template argument

Copy link
Contributor

@0x6e 0x6e left a comment

Choose a reason for hiding this comment

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

As it stands, the attached property is only going to update the widget stretch when the widget is added to the Splitter. Would it not be better to have the attached property directly read and write from the widget size policy?

@Znurre
Copy link
Contributor Author

Znurre commented Mar 28, 2018

Okay, I think I have fixed everything.
I decided to drop the attached property in favor of the new sizePolicy property on DeclarativeWidgetExtension, seeing as that works just as well.

Also, please note that there is some inconsistency in the formatting and the dates in the copyright headers.

Some parts of the project use 2 space indent, while others (like the test cases) use 4 space indent.
I have tried to match the surrounding formatting, but it would be good to know what is preferred.

Also, some headers simply have "2017" as date, mainly the test parts of the code, while others have "2013-2017".
I have tried to match this as well, but as I said, I would really appreciate some hints on what is preferred :)

@krake-kdab
Copy link
Contributor

I decided to drop the attached property in favor of the new sizePolicy property on DeclarativeWidgetExtension, seeing as that works just as well.

Great!

Some parts of the project use 2 space indent, while others (like the test cases) use 4 space indent.
I have tried to match the surrounding formatting, but it would be good to know what is preferred.

Yes, the original 2 space indent came from being a "private playground project" from before there was a Qt indentation standard.
I would say use 4 spaces for new files like elsewhere in Qt, but keep consistency in old files.

Also, some headers simply have "2017" as date, mainly the test parts of the code, while others have "2013-2017".

I would say just the current year for new files

Copy link
Contributor

@krake-kdab krake-kdab left a comment

Choose a reason for hiding this comment

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

Looks like DeclarativeSplitter is no longer needed

@Znurre
Copy link
Contributor Author

Znurre commented Mar 29, 2018

You're right, I have removed it now.
Also thank you for the clarification regarding formatting.
I'll make sure to stick to the Qt guidlines for new files in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants