About dialog & export dialog improvements. #3826

Sawuare · 2017-09-25T16:06:23Z

Changes:
• Improved code style.
• Improved translatable strings.
• Improved the geometry of the dialogs.
• Improved the position of some items in the dialogs.

TODO:
• In the about dialog, make the color of the link the same as the color of the strings in the tabs.
I couldn't find the exact color code...

• Make the about dialog a bit shorter to fit its content more nicely.

Spekular · 2017-09-26T05:33:59Z

I'm not a fan of the about dialog changes, but the rest looks great!

…

On Sep 25, 2017 18:06, "Hussam Eddin Alhomsi" ***@***.***> wrote: *Changes:* • Improved code style. • Improved translatable strings. • Improved the geometry of the dialogs. • Improved the position of some items in the dialogs. *The about dialog changes are small.* *TODO:* • More tests. About dialog (before) About dialog (after) [image: ad before] <https://user-images.githubusercontent.com/25503477/30818428-311638f8-a224-11e7-93ef-07daac7fc56e.png> [image: ad after] <https://user-images.githubusercontent.com/25503477/30818436-38aa0536-a224-11e7-83c3-f568972a15b0.png> Export dialog (before) Export dialog (after) [image: ed before] <https://user-images.githubusercontent.com/25503477/30818228-a63148ea-a223-11e7-8314-233548c7bf62.png> [image: ed after] <https://user-images.githubusercontent.com/25503477/30818236-ab343640-a223-11e7-95e2-09fe87c94e4e.png> ------------------------------ You can view, comment on, or merge this pull request online at: #3826 Commit Summary - Initial commit. - Fixes. File Changes - *M* CMakeLists.txt <https://github.com/LMMS/lmms/pull/3826/files#diff-0> (2) - *M* src/core/ProjectRenderer.cpp <https://github.com/LMMS/lmms/pull/3826/files#diff-1> (162) - *M* src/gui/AboutDialog.cpp <https://github.com/LMMS/lmms/pull/3826/files#diff-2> (34) - *M* src/gui/ExportProjectDialog.cpp <https://github.com/LMMS/lmms/pull/3826/files#diff-3> (146) - *M* src/gui/dialogs/about_dialog.ui <https://github.com/LMMS/lmms/pull/3826/files#diff-4> (32) - *M* src/gui/dialogs/export_project.ui <https://github.com/LMMS/lmms/pull/3826/files#diff-5> (76) Patch Links: - https://github.com/LMMS/lmms/pull/3826.patch - https://github.com/LMMS/lmms/pull/3826.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3826>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIgVmqaKY9GF4MbJ0wA-TYQyDNTZujMKks5sl8-HgaJpZM4Pi-L8> .

Sawuare · 2017-09-26T11:28:45Z

Are there specific about dialog changes that you don't like?

Spekular · 2017-09-26T14:41:16Z

Changing the BG to the same everywhere means your eyes are less drawn to the relevant portion of the window. The tabs also look awkward now.

Also, I hadn't checked these at full resolution, but I'm not too crazy about the "depth" you've added on the outlines, and especially not on the progress bar. This looks further from the single window/flat look we're steering towards than it is now.

tresf · 2017-09-26T14:57:03Z

The current gray color is terrible for text because it is neither dark or light enough for text contrast. It's also inconsistent with the rest of the UI currently. I'm in agreement with @Sawuare, it needs to be darker. If it needs to be a different shade, we can cherry-pick one from the rest of the UI. I would use the effects chain as an example of how to show a border, depth and content inside a container.

@Spekular please try to be constructive in your comments so that @Sawuare can have concrete changes to make.

Spekular · 2017-09-26T15:29:09Z

your eyes are less drawn to the relevant portion of the window

This looks further from the single window/flat look we're steering towards than it is now.

I fail to see how these aren't constructive.

As for the tabs, I think my issue is that the tabs in the background are brighter than those in the foreground, confusing the matter of which is in focus (especially since text is the same color on both). Also, our other tab-like elements (sidebar, tabs in instruments, tabs in settings) have the same size tabs for active and inactive. The large height difference in the active tab looks out of place in comparison, especially now that the about section is vertically shorter.

In addition, the export dialog sections are inset (thicker line on top indicates shadow from light source above), whereas the sections of the about screen pop out (drop shadow below them).

We've made decisions in the past based around moving towards the look set forth in budislav's single window concept, and the constant width of the previous borders is more in line with that look than the inset/popping ones in this PR.

Sawuare · 2017-09-26T15:29:54Z

The BG color wasn't changed by me, but I like it.
The outlines weren't changed by me either, but I also like them. 😃

Spekular · 2017-09-26T15:34:05Z

Then your before/after screenshots are giving a bad comparison, no? If this PR isn't changing the outline it should be the same in both.

Sawuare · 2017-09-26T16:36:34Z

Then your before/after screenshots are giving a bad comparison, no? If this PR isn't changing the outline it should be the same in both.

You're right. The before pictures are from RC3 😅, I will update them to be from master.

Spekular · 2017-09-26T16:46:40Z

Great, thanks!

Spekular · 2017-09-26T18:39:12Z

As far as I can tell, all that changed is that the windows are smaller and the export dialogue checkboxes were moved out from "quality settings". Is this correct? If so I have no objections.

tresf · 2017-09-26T18:40:59Z

The visual differences may be due to the installed (Qt5) version versus the compiled (Qt4) version. We don't have instructions written to compile against Qt5 yet on windows.

tresf · 2017-09-26T18:49:44Z

CMakeLists.txt

@@ -22,7 +22,7 @@ STRING(TOUPPER          "${CMAKE_PROJECT_NAME}" PROJECT_NAME_UCASE)
 # Updated by maintenance tasks
 SET(PROJECT_YEAR 2015)

-SET(PROJECT_AUTHOR      "LMMS Developers")
+SET(PROJECT_AUTHOR      "LMMS developers")


Why lowercase here? Is this necessary change as part of this PR or was it something that was never reverted after testing?

It's the latter. I will capitalize it.

tresf · 2017-09-26T18:51:59Z

src/core/ProjectRenderer.cpp

-		QT_TRANSLATE_NOOP("ProjectRenderer", "FLAC-File (*.flac)"),
-		".flac",
-		&AudioFileFlac::getInst
+	{ProjectRenderer::WaveFile,


We need to stop translating stuff like *.wav, *.mp3. This is only going to cause issues down the road where a bad translation breaks a feature.

Yeah according to #3677 this is already making problems in other places

I just removed the space after { here.
Do you want me to keep the strings but make them untranslatable? If so, is removing QT_TRANSLATE_NOOP enough?

tresf · 2017-09-26T18:55:42Z

@Sawuare I'm going to be a bit critical here and other devs feel free to chime in and refute, but I really don't see the benefit of doing the code cleanup in this PR. If this PR is about condensing the UI, that's what it should do and only that. There are somewhat unrelated and random changes here such as formatting, removing "Compressed" from the MP3 and OGG file extensions and they're hard to review when the formatting has changed so much.

Also as @Spekular has pointed out above, the before and after screenshots are bad examples of the changes due to the mixed Qt versions.

We all want the formatting to get cleaner in the codebase, but I think that's a candidate for a separate all-breaking PR that should stand on its own.

Umcaruje · 2017-09-26T18:58:37Z

We all want the formatting to get cleaner in the codebase, but I think that's a candidate for a separate all-breaking PR that should stand on its own.

This. @tresf said it all, don't have anything to add. I feel the same way.

gi0e5b06 · 2017-09-27T10:38:19Z

@tresf Sure. A dozen of PR would certainly be useful. In fact, one PR for each changed line of code should be the standard.

@Sawuare Good work. Let's merge and move on.

BTW, I'm going to add two checkboxes in the export dialog:

remove silence (at the beginning and the end)
normalize audio output (peak)

Sawuare · 2017-09-27T11:27:06Z

I'm going to reverse all formatting changes and remove the before and after images. 👍

@gi0e5b06
Your suggested check boxes are a great idea, but doing them in a separate PR is the best thing to do. 😉

Sawuare · 2017-09-27T11:31:46Z

@tresf

removing "Compressed" from the MP3 and OGG file extensions and they're hard to review when the formatting has changed so much.

You're right about the formatting stuff, but I removed "Compressed" and other stuff because I think they belong in the documentation (when it's created), not in the program itself.

Umcaruje · 2017-09-27T12:07:01Z

Let's merge and move on.

I'm sorry but you are in no position to say that.

BTW, I'm going to add two checkboxes in the export dialog:

remove silence (at the beginning and the end)
normalize audio output (peak)

You do realize those changes will never make it into the codebase if you don't play by the rules? We can discuss all you want, but one PR per feature is how things are done here, and the majority of the community agrees on it.

Umcaruje · 2017-09-27T12:38:27Z

src/gui/AboutDialog.cpp


-	licenseLabel->setPlainText( embed::getText( "LICENSE.txt" ) );
+	/*involvedLabel->setPlainText(


Why did you disable the contributors file, it contains all of our github contributors and it's more accurate than the authors file (though the authors file contains some people that wouldn't be credited through github)

it contains all of our github contributors

@Umcaruje Not anymore, jasp00 removed the CMake logic that used to create the file in 30f1e52. He planned to replace that by a maintenance task on our server, but it appears it was never implemented correctly. That's probably why @Sawuare removed it. Related: LMMS/lmms.io/pull/198, #3016

Then, should we update the authors file with some missing people? Or revert @jasp00's changes, since he sent a mail saying he won't be able to contribute for some time.

I didn't know the story of the contributors tab. I just removed it because it didn't display anything other than: "Contributors ordered by number of commit:".

tresf · 2017-09-27T13:10:17Z

I'll take over the CONTRIBUTORS file before stable-1.2, just backlogged on the task. Please keep it in the codebase @Sawuare.

I removed "Compressed" and other stuff because I think they belong in the documentation (when it's created), not in the program itself.

After typing that I took a look at some other programs and they don't use the word "Compressed" either. From a translation perspective, and it's more to translate. Removing it seems warranted.

A dozen of PR would certainly be useful

@gi0e5b06 No, that would not be useful.

one PR for each changed line of code should be the standard.

Although I find this "revolting defiance" to be amusing, it's better left in our Discord chat server. There are some pros and cons to "reformatting as you go" and we can talk about that on Discord if it is going to be a problem with contributions. In this case, @Sawuare is making his first PR, so we're trying to set reasonable standards moving forward, but they aren't set in stone. https://lmms.io/chat, then click on #devtalk.

lukas-w · 2017-09-27T13:24:53Z

src/core/ProjectRenderer.cpp

 		".flac",
 		&AudioFileFlac::getInst
 	},
 	{ ProjectRenderer::OggFile,
-		QT_TRANSLATE_NOOP( "ProjectRenderer", "Compressed OGG-File (*.ogg)" ),
+		QT_TRANSLATE_NOOP( "ProjectRenderer", "OGG (*.ogg)" ),


While we're at it, can we make this say "Ogg-Vorbis" instead? This would correctly reflect the fact that Ogg is merely the container format, while Vorbis is the audio codec that's being used here.

From wikipedia.org:

Before 2007, the .ogg filename extension was used for all files whose content used the Ogg container format. Since 2007, the Xiph.Org Foundation recommends that .ogg only be used for Ogg Vorbis audio files. The Xiph.Org Foundation decided to create a new set of file extensions and media types to describe different types of content such as .oga for audio only files, .ogv for video with or without sound (including Theora), and .ogx for multiplexed Ogg.[4]

"OGG" should be just fine.

Sawuare · 2017-09-30T09:46:04Z

The "Export as loop" option does not work, I'll comment it out if there are no objections.

zonkmachine · 2017-09-30T09:55:54Z

I'll comment it out if there are no objections.

Don't. If it doesn't work it's a bug and should be fixed. Is this bug in master only?

Sawuare · 2017-09-30T10:08:41Z

Is this bug in master only?

It is in RC.3 and master.

zonkmachine · 2017-09-30T10:21:52Z

It is in RC.3 and master.

OK. I'll have a look.

zonkmachine · 2017-09-30T10:39:33Z

Tested. The option marked 'Export as loop (remove end silence)' works just fine in 1.2.0-rc3.57. Can you please describe steps to reproduce the issue?

Sawuare · 2017-09-30T11:04:14Z

To reproduce:
1, Create a pattern for any instrument.
2. Put 2 8th notes after each other.
3. Export with the 'Export as loop (remove end silence) option.

There will be silence after the notes.

zonkmachine · 2017-09-30T11:47:37Z

Not a bug.

Export as loop and default export together.

By default an extra bar is added to allow for notes and delay effects to end. You can choose to export as a loop but it will not end the sample after the right note but still fill out the whole last bar.

Here is a recent issue suggesting an option to remove silence. #3830

Sawuare · 2017-09-30T12:04:05Z

Thank you for clarifying.

Perhaps we change the option's name to something clearer, like "Export as loop (remove extra bar)"?

tresf · 2017-11-15T04:57:36Z

Looks good.

A follow-up to PR #3826.

* Improve appearance of about dialog, export dialog * Cleanup code comments/formatting

A follow-up to PR LMMS#3826.

Hussam Eddin Alhomsi added 2 commits September 25, 2017 18:19

Initial commit.

1c31bb5

Fixes.

d1e2815

Hussam Eddin Alhomsi added 2 commits September 26, 2017 15:10

Some about dialog changes.

b1cc942

Change color of link to something green-ish.

b2ea361

Commeneted out useless code.

0404639

tresf reviewed Sep 26, 2017

View reviewed changes

Formatting reverse.

1da95ba

Umcaruje reviewed Sep 27, 2017

View reviewed changes

Hussam Eddin Alhomsi added 2 commits September 27, 2017 15:55

Formatting reverse 2.

015f4c9

Formatting reverse 3.

080c8b4

lukas-w reviewed Sep 27, 2017

View reviewed changes

Return "Involved" tab. Revert AboutDialog.cpp changes.

4398747

Commented out "Export as loop".

351c8ca

Undo commented out "Export as loop".

10f4924

Umcaruje added this to the 1.3.0 milestone Oct 2, 2017

Hussam Eddin Alhomsi added 3 commits October 26, 2017 17:37

ED: (Remove end silence) -> (Remove extra bar).

9da7ae6

Export dialog should not be resizable.

bb43f97

"LMMS developers" to "LMMS Developers".

aa589fa

tresf merged commit 548baa5 into LMMS:master Nov 15, 2017

Sawuare deleted the ADEDI branch November 15, 2017 12:13

Sawuare pushed a commit that referenced this pull request Nov 16, 2017

Remove useless code from the about & export dialogs.

aad98d7

A follow-up to PR #3826.

PhysSong mentioned this pull request Jan 6, 2021

MP3 stereo mode doesn't match to the selection in export dialog #5876

Closed

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

About dialog & export dialog improvements. (LMMS#3826)

4471aae

* Improve appearance of about dialog, export dialog * Cleanup code comments/formatting

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Remove useless code from the about & export dialogs.

d5a6ca0

A follow-up to PR LMMS#3826.


		licenseLabel->setPlainText( embed::getText( "LICENSE.txt" ) );
		/*involvedLabel->setPlainText(

About dialog & export dialog improvements. #3826

About dialog & export dialog improvements. #3826

Conversation

Sawuare commented Sep 25, 2017 • edited Loading

Spekular commented Sep 26, 2017 via email

Sawuare commented Sep 26, 2017

Spekular commented Sep 26, 2017

tresf commented Sep 26, 2017

Spekular commented Sep 26, 2017 • edited Loading

Sawuare commented Sep 26, 2017

Spekular commented Sep 26, 2017

Sawuare commented Sep 26, 2017

Spekular commented Sep 26, 2017 via email • edited Loading

Spekular commented Sep 26, 2017

tresf commented Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf commented Sep 26, 2017

Umcaruje commented Sep 26, 2017

gi0e5b06 commented Sep 27, 2017

Sawuare commented Sep 27, 2017

Sawuare commented Sep 27, 2017

Umcaruje commented Sep 27, 2017

Choose a reason for hiding this comment

lukas-w Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf commented Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sawuare commented Sep 30, 2017

zonkmachine commented Sep 30, 2017

Sawuare commented Sep 30, 2017

zonkmachine commented Sep 30, 2017

zonkmachine commented Sep 30, 2017 • edited Loading

Sawuare commented Sep 30, 2017

zonkmachine commented Sep 30, 2017

Sawuare commented Sep 30, 2017

tresf commented Nov 15, 2017

Sawuare commented Sep 25, 2017 •

edited

Loading

Spekular commented Sep 26, 2017 •

edited

Loading

Spekular commented Sep 26, 2017 via email •

edited

Loading

tresf commented Sep 26, 2017 •

edited

Loading

tresf Sep 26, 2017 •

edited

Loading

lukas-w Sep 27, 2017 •

edited

Loading

tresf commented Sep 27, 2017 •

edited

Loading

zonkmachine commented Sep 30, 2017 •

edited

Loading