FashionFreedom / Seamly2D

Open source patternmaking software to democratize fashion.
https://seamly.io
GNU General Public License v3.0
616 stars 111 forks source link

Background image feature MVP #1085

Closed Onetchou closed 7 months ago

Onetchou commented 7 months ago

MVP for the Issue #440

The user can add background images to the scene. Those images are not saved into the pattern file in this first version of the feature and they are not affected by the "undo".

The user can interact with those images and change their properties : image

closes issue #440

CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

DSCaskey commented 7 months ago

@Onetchou

Can't build the PR... there's conflicts in the ts files (as well as few other minor conflicts). Not sure what happened, but I got the same conflict warning awhile ago when I tried to fetch changes on your local branch. There's 100's of conflicts in each ts, so I think you're going to need to copy the ts files from the main or your develop and rerun lupdate.

I found it best to merge / rebase current develop into the local feature before pushing the branch and a making a PR. It's avoid having conflicts in the PR.

DSCaskey commented 7 months ago

@Onetchou

Also to note: The updated dialog is all wonky now. The labels don't line up vertically anymore which I suspect is from not putting the changed widgets in the proper layout(s). And I'm not sure why there is now a big space between the labels: and the widgets - except for 2 the 2 lock ones that are correct? I'm wondering too if maybe the Lock Image might not be better placed in the Selection group at the top?

imge_dialog

Onetchou commented 7 months ago

The labels don't line up vertically anymore which I suspect is from not putting the changed widgets in the proper layout(s).

I think the issue is only the "Lock Image" which is not aligned with the "Unit:" and "Lock aspect ratio:" labels.

Aligning the 3 "label + button" together in the center seemed a good solution. The buttons are aligned together and the labels should be aligned together on their left side.

image

And I'm not sure why there is now a big space between the labels: and the widgets

It was in order to keep the above alignement, because with the buttons aligned, since the labels have different length it seemed very messy, we spent a lot of time trying to find a proper layout but it seems that it is still not perfect haha. The issue is not "technical" but more of a "GUI design" one 😉

I'm wondering too if maybe the Lock Image might not be better placed in the Selection group at the top?

Good idea, I'll try!

Onetchou commented 7 months ago

Now it should be better: image

Onetchou commented 7 months ago

Can't build the PR... there's conflicts in the ts files (as well as few other minor conflicts). Not sure what happened, but I got the same conflict warning awhile ago when I tried to fetch changes on your local branch. There's 100's of conflicts in each ts, so I think you're going to need to copy the ts files from the main or your develop and rerun lupdate.

@DSCaskey The conflicts have been solved, and the misalignment in the dialog has been corrected 😉

DSCaskey commented 7 months ago

@Onetchou

I'm going to merge the PR, but I'd like to address the dialog in the advanced revision. Here's my reasoning.

For starters the length of the label text "Lock aspect ratio" can be reduced to "Lock aspect". "Ratio" is superflous. Secondly there are basically 4 styles for a Form Layout:

Windows... where the Label is left justified and the field is left justied, and all fields expand to fit the space.

Screenshot 2024-04-14 192714

MacOS... where the Labels are right justified, fields are left justfied, fields only expand to size hint. Screenshot 2024-04-14 192722

KDE... similar to MacOS, where some fields expand to fit space Screenshot 2024-04-14 192731

Qt Extended... Labels are right aligned, fields are left aligned, and all fields expand to fit space. Screenshot 2024-04-14 192738

IMO the Windows style is harder to read as the labels are not directly connected to the fields as there is space between them - which only gets worse the greater the difference between the label lengths in a layout is. Which is why I prefer the MacOs style, but I prefer the fields expand to fill the space. Thus the Qt Extended styling.

What you have created is a "Qt Jenga" style that fits none of the above.

I took a few minutes to redo the layout... BTW... I never said that the dialog needs to be "Fixed" in size... but rather the Minimum size can be increased to 350, and the Min label width to 100. I also think the field content should be left justified.

Screenshot 2024-04-14 015134

Here's what it currently is... and this is what happens to the dialog if we add the sizegrips.. the lock & unit buttons get even more Jenga'd. Note though it does look better with the field content left justified. Also you may noticed that you removed the vertical spacer which keeps the Group boxes from expanding to fit the space... the Attributes Groupbox has all this extra space that doesn't look right.

image