Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Background image feature MVP #1085

Merged
merged 79 commits into from
Apr 15, 2024
Merged

Background image feature MVP #1085

merged 79 commits into from
Apr 15, 2024

Conversation

Onetchou
Copy link
Contributor

@Onetchou Onetchou commented Apr 12, 2024

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

DSCaskey and others added 30 commits August 7, 2022 18:10
Adds the ability to load images onto the background of the draft scene.
"MainWindow::handleImageSelected" has been kept for the future development of this feature
Now ctrl is used to resize the images from their center points
Visibility and origin point options have been hidden.
…pect ratio if the corresponding option was set in image properties
Minor refactoring of the code of the delete button
@CLAassistant
Copy link

CLAassistant commented Apr 12, 2024

CLA assistant check
All committers have signed the CLA.

@DSCaskey
Copy link
Contributor

DSCaskey commented Apr 12, 2024

@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
Copy link
Contributor

DSCaskey commented Apr 13, 2024

@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
Copy link
Contributor Author

Onetchou commented Apr 13, 2024

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
Copy link
Contributor Author

Onetchou commented Apr 13, 2024

Now it should be better:
image

@Onetchou
Copy link
Contributor Author

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
Copy link
Contributor

@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

@DSCaskey DSCaskey merged commit 582f3ee into develop Apr 15, 2024
10 checks passed
@DSCaskey DSCaskey deleted the background-image branch April 15, 2024 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NEW UI/WORKFLOW: Import image file as a background
4 participants