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

Fix overlapping regions in the track properties editor #1366

Merged
merged 20 commits into from
Nov 4, 2017

Conversation

esbrandt
Copy link
Contributor

See https://bugs.launchpad.net/mixxx/+bug/1708910. Fixed some other quirks as well.
Screen shoot shows the fixed track editor (resized to HMinimum) on MacOS, please test on other OS as well

After:
public enemy - nothing is quick in the desert

…his has been used in the past as band-aid for aligning the field in the grid layout, it did not prevent fields from overlapping when resizing the widget though.
…label some points down. This label is aligned to the top, and was displayed a little off vertically from the multi-line text field next to it.
… editable fields get as much space as possible, while the right part of the grid (cover image) is resizable on a smaller scale.
…d to ``Ignore``. This is the only way i found to NOT have the fields above overlap on resize. Add a minimal size, approx. 2 lines of text. This should be enough even for long file name/file path. Make sure the field stretches on resize too.
…er to the right when resizing. Previously it was centered to the then fixed text fields below.
…tim from the option in the beat detection preferences, so the Transifex translation memory can pick up and auto-fill the string if available in the respective language.
…alphabetically. Buttons should get focus too when tabbing, but this does apparently not work (macOS).
@daschuer
Copy link
Member

Than you for adopting this. It is better now, but there are some remaining issues. See:
bildschirmfoto vom 2017-10-29 22 03 44

  • The metadta buttons are trunkated.
  • "Location:" The colon is not aligned with the lines above. The location itself sits to low.
  • Is it necessary to scale (change) the track metadata rows high, I think it can be fixed.

@uklotzde
Copy link
Contributor

uklotzde commented Oct 29, 2017

👍 I tried this once, but then gave up very soon ;)

I also noticed that the label on buttons is still truncated. But we need to rethink those buttons anyway and the text length depends on the display language. We could split them into 2 groups and avoid redundant text:

Track Metadata: [Get from MusicBrainz] [Reload from File] | [Open in File Browser]

I further noticed that the alignment of the cover image could be improved (Fedora, Gnome, Fonts 11 pt, Scaling 1.00). The appearance is very similar to the screenshot that Daniel posted.

minsize

resized

@esbrandt
Copy link
Contributor Author

Thanks for testing, i committed a possible fix for the button/row resizing issues.

How would you expect the cover to align when resizing? Pinned to the right (current), centered above, neither of the two?

Re: last screenshoot
Do we need a little more space in the right column ? Never noticed that more than four digits are allowed in the year field.

I have no solution yet for the magic margin of the locations QPlaintextEdit, can´t we use a QLabel and add a wordWrap?

@uklotzde
Copy link
Contributor

Cover image: Centered inside its bounding box, using as much of the available space as possible. Currently their seems to be a spacer at the top that is also limiting further horizontal scaling by preserving the aspect ratio.

@uklotzde
Copy link
Contributor

The name 'year' for this field has historical reasons. It may contain year, year + month, date, date + time (ISO-8601).

I agree, it is annoying to see only minutes and seconds or the instead of the year ;) Maybe we could change the alignment from right to left bound? Never thought about this.

@esbrandt
Copy link
Contributor Author

esbrandt commented Nov 3, 2017

Thanks for the comments, here´s what´s cooking:

public enemy - nothing is quick in the desert-1

  • Editable, and non-editable fields moved into a separate Groupbox.

  • Wider right column in the Details groupbox. Allows more characters in the year field.

  • Cover centered when resizing the modal window. Fixed size cover here, otherwise it will throw off the rows in the grid.

  • Location is now a label. Previous it was a plaintextedit, and gave some issues with margins and alignments.

  • Only 2 columns for the File groupbox, makes easier to read. No more mixed font weights. IMO all available data should be visable here, same as in the track table ( date added, modified... etc.)

  • Move action buttons into the related groupboxes.

  • I wonder if Key should move to the non-editable fields too. Or it is just unexpected behavior/ a bug: If you manually change from the default key ( determined by the analyzer) to a different key, and then analyze the file again, the key stays at the custom key. This works different for the BPM field. So you might end up with a wrong key in the database, with no way to reset.

Comments?

@uklotzde
Copy link
Contributor

uklotzde commented Nov 3, 2017

There is a bug in AnalyzerKey, m_bPreferencesReanalyzeEnabled is not read from the settings. Analysis results are ignored unless you reset the key manually to an empty string.

@uklotzde
Copy link
Contributor

uklotzde commented Nov 3, 2017

@esbrandt Did you forget to push? The branch has not been updated.

* Editable, and non-editable fields moved into a separate Groupbox.
* Wider right column in the ``Details`` groupbox. Allows more characters in the ``year`` field.
* ``Cover`` centered when resizing the modal window. Fixed size cover here, otherwise it will throw off the rows in the grid.
* ``Location`` is now a label. Previous it was a plaintextedit, and gave some issues with margins and alignments.
* Only 2 columns for the ``File`` groupbox, makes easier to read. No more mixed font weights.
* Move action buttons into the related groupboxes.
@uklotzde
Copy link
Contributor

uklotzde commented Nov 4, 2017

  • Separating the track metadata from the file properties is a good idea. A horizontal line might be helpful for this separation, now it looks like a lot of wasted space.
  • The cover image is framed by 3 black lines, but not on the bottom. I don't think that this is intended.
  • The space above the cover image is still wasted. When increasing the size of the dialog the cover image stays almost the same size, although there is plenty of space around it.
  • The minimum size constraints are not respected when resizing the dialog back and forth.

2017-11-04 06-56-55 default size
2017-11-04 06-56-07 increased size
2017-11-04 06-55-44 broken minimum size

…ue, where the cover box did not align to the top of the gridlayout. The cover takes as much horizontal space as allowed by the input fields next to it. It preserves aspect ratio when scaling vertically.
…xed usage for labels in other parts of the preferences, which will be cleaned up in a later PR. Our translation memory will take of the translation automatically, since we use the strings (- colon) already for the track table headers.
… allowed to resize to zero. This was no issue on macOS, but at least on Linux with Gnome.
@esbrandt
Copy link
Contributor Author

esbrandt commented Nov 4, 2017

Please test again, hopefully the covers alignment, and the resize issue are fixed.

Re: Wasted space between groupboxes. We use the same kind of groupbox layout in other parts of the preferences ( e.g. live broadcasting). Gnome ( i guess thats whats in the screenshot) decided to display groupboxes flat, even if the are supposed to be decorated (see macOS screenshot).

@uklotzde
Copy link
Contributor

uklotzde commented Nov 4, 2017

Fixed

  • Resizing works as expected, minimum size is respected
  • Cover image is now displayed symmetrically

Minor Issues

  • Tab order: One invisible item/stop between the buttons "Open In File Browser" and "Previous"
  • Tab order: I would expect the tab order Title...Grouping followed by Year...Track #. Currently it is alternating horizontally after visiting the first 4 rows in vertical order.

Wishlist

  • I would like to see less padding between/below the Details and the File sections
  • Less padding around the cover image would be very helpful. See my attached screenshot where I marked both the available (orange) and actual (green) extend for the cover box without any padding.
  • In the BPM tab a horizontal separator is displayed between the controls and the "Hint: ..." label. This is what I expected between the Details and the File sections here.

screenshot from 2017-11-04 13-32-24

@esbrandt
Copy link
Contributor Author

esbrandt commented Nov 4, 2017

Fixed

Resizing works as expected, minimum size is respected
Cover image is now displayed symmetrically

👍

Minor Issues

Tab order: One invisible item/stop between the buttons "Open In File Browser" and "Previous"

Looks like a platform specific issue. Tabbing does not work for buttons at all on macOS. I can only tab between the editable fields. In the code, the tab order went from Open in file browser to the text field in the Comments tab, and then further to the BPM tab.

Tab order: I would expect the tab order Title...Grouping followed by Year...Track #. Currently it is alternating horizontally after visiting the first 4 rows in vertical order.

Fixed for the summary tab. Still different with the other tabs, requires re-grouping of buttons in the BPM tab eventually. Should check other preferences pages too ( not gonna happen in this PR)

Wishlist

I would like to see less padding between/below the Details and the File sections

The groupboxes use the default values here, touching them would mean to me changing them in all other pages they occur, basically all over the preferences pages.

Less padding around the cover image would be very helpful. See my attached screenshot where I marked both the available (orange) and actual (green) extend for the cover box without any padding.

Platform specific issue again,fine on macOS. We could use some feedback from windows, and decide. The cover label is drawn to a horizontal box which sits inside a widget, which both apply margins eventually. Is there a need for the extra groupbox? Could use some help with this.

In the BPM tab a horizontal separator is displayed between the controls and the "Hint: ..." label. This is what I expected between the Details and the File sections here.

There is a horizontal line because this gives some kind of separation for groups of items, that are NOT grouped into Groupboxes.

Again we face the platform specific issues mentioned before, adding a horizontal line here would feel awkward with other OS.

@uklotzde
Copy link
Contributor

uklotzde commented Nov 4, 2017

Tab order now works as expected. Platform-specific layout issues can be figured out later. Upgrading to Qt 5 will change things anyway.

👍 LGTM. I vote for merging this PR, although I haven't tested it on Windows.

@daschuer
Copy link
Member

daschuer commented Nov 4, 2017

This is the minimum size Windows 10 Version:
bildschirmfoto vom 2017-11-04 22-05-41

LGTM Thank you.

@daschuer daschuer merged commit c8e724e into mixxxdj:master Nov 4, 2017
@esbrandt esbrandt deleted the lp1708910 branch January 27, 2018 14:19
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.

None yet

3 participants