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

Allow WidgetTab to use artwork tabs (@StCyr) #3569

Merged
merged 39 commits into from May 31, 2017

Conversation

Projects
None yet
3 participants
@Umcaruje
Member

Umcaruje commented May 21, 2017

This is an effort to bring #2599 to a closure. The original PR went stale, and most of the work has been done, so it'd be a shame for it to never get implmeneted, as I think it's a nifty feature.

I rebased the original commits to stable-1.2 and will address the remaining issues. I couldn't push to @StCyr's branch cause I don't have access and github didn't have the feature to allow edits form mantainers at the time of that PR.

Here's how it looks now:
image
I'll be changing the colors shortly. @RebeccaLaVie input appreciated.

Cyrille Bollu and others added some commits Feb 12, 2016

First version of artwork tabs for the InstrumentTrackWindow.
This version can only display & manage artwork tabs, which breaks
the InstrumentSoundShapingView as it still uses text tabs.

I'm planing to improve this implementation to let these artwork tabs fall back
to text mode when no artwork is given. This would solve the problem of the
InstrumentSoundShapingView.
Second version of artwork tabs for the InstrumentTrackWindow.
This version will draw an artwork tab when the TabWidget::addTab function is given
a pixmapName. Otherwise, when pixmapName is NULL, it will fall back drawing a text
tab.
Added tooltip support for the TabWidget class.
Atm, tooltips are simply tabs' name.
1) Tabs in TabWidget class have now a "tooltip" attribute. So that th…
…ey can now show

more meaningfull information then simply the tab's name.
2) Fixed the compilation problem with QT5
Increased the graphical TabWidget's height by 2 pixels for eye-candy.
The InstrumentTrackWindow's height has been increased by the same amount.

Cyrille

Cyrille Bollu added some commits Aug 2, 2016

Revert "TabWidget: Made the artworks' color themeable"
This reverts commit 5b162c0.

Conflicts:
	src/gui/widgets/TabWidget.cpp

Reason: Artwork's color themeability had the side-effect that it removed the artworks' alpha
channel, thus making them ugly.

@Umcaruje Umcaruje added this to the 1.2.0 milestone May 21, 2017

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 21, 2017

Member

Adressed @tresf and @jasp00 's comments from the original PR. The name is used as a tooltip without a seperate variable. The names are as follows:

  1. Plugin
  2. Envelope, filter & LFO
  3. Chord stacking & arpeggio
  4. Effects
  5. MIDI settings
  6. Miscellaneous
Member

Umcaruje commented May 21, 2017

Adressed @tresf and @jasp00 's comments from the original PR. The name is used as a tooltip without a seperate variable. The names are as follows:

  1. Plugin
  2. Envelope, filter & LFO
  3. Chord stacking & arpeggio
  4. Effects
  5. MIDI settings
  6. Miscellaneous
@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 21, 2017

Member

After a session with the community on discord, this is how the tabs look now:
image

There are some spacing issues that I'd like to address tomorrow.

Member

Umcaruje commented May 21, 2017

After a session with the community on discord, this is how the tabs look now:
image

There are some spacing issues that I'd like to address tomorrow.

@qnebra

This comment has been minimized.

Show comment
Hide comment
@qnebra

qnebra May 27, 2017

Tested and works nicely, not causing errors on usage.

qnebra commented May 27, 2017

Tested and works nicely, not causing errors on usage.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 30, 2017

Member

@LMMS/developers anyone want to review the code on this? Would be appreciated

Member

Umcaruje commented May 30, 2017

@LMMS/developers anyone want to review the code on this? Would be appreciated

Show outdated Hide outdated include/TabWidget.h
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf May 30, 2017

Member

@Umcaruje this looks pretty good. I haven't reviewed it for any syntax errors, but the change appears to be quite minor and straightforward despite the number of files.

The major change I see is the draw code has moved from resize event to paint event which may have slight performance implications, but the draw routines are greatly simplified as well. Unfortunately I don't have the experience with updating paint routines to know where they should go from a design perspective.

Member

tresf commented May 30, 2017

@Umcaruje this looks pretty good. I haven't reviewed it for any syntax errors, but the change appears to be quite minor and straightforward despite the number of files.

The major change I see is the draw code has moved from resize event to paint event which may have slight performance implications, but the draw routines are greatly simplified as well. Unfortunately I don't have the experience with updating paint routines to know where they should go from a design perspective.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 30, 2017

Member

The major change I see is the draw code has moved from resize event to paint event which may have slight performance implications, but the draw routines are greatly simplified as well. Unfortunately I don't have the experience with updating paint routines to know where they should go from a design perspective.

Well those were @StCyr's changes, and it makes sense to me to draw in the paint event. Anyways I didn't notice any performance impacts and neither did @qnebra from what I've gathered.

This looks ready to merge then. Will fix the comment formatting.

Member

Umcaruje commented May 30, 2017

The major change I see is the draw code has moved from resize event to paint event which may have slight performance implications, but the draw routines are greatly simplified as well. Unfortunately I don't have the experience with updating paint routines to know where they should go from a design perspective.

Well those were @StCyr's changes, and it makes sense to me to draw in the paint event. Anyways I didn't notice any performance impacts and neither did @qnebra from what I've gathered.

This looks ready to merge then. Will fix the comment formatting.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 30, 2017

Member

image
Here's the final look of the icons, realised I didn't post a screenshot before

Member

Umcaruje commented May 30, 2017

image
Here's the final look of the icons, realised I didn't post a screenshot before

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 30, 2017

Member

I'll leave this sitting for 24 hours, and merge after that.

Member

Umcaruje commented May 30, 2017

I'll leave this sitting for 24 hours, and merge after that.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf May 30, 2017

Member

it makes sense to me to draw in the paint event.

This is the part I'm a bit confused about as it was working before and only being triggered on construction or resize.

But the way he made it is more consistent with how we do it today in other classes, so it's only an observation.

To that point, calling update() on construction seems quite redundant, no? It made sense before when the constructor wasn't using paintEvent, but now that it is it should be called naturally as part of the widget constructor. It's not harmful update() can be called just about anywhere, but I think it's redundant. If you look at other classes (e.g. Fader, PixmapButton that override paintEvent(), they do not call update() in the constructor)

Member

tresf commented May 30, 2017

it makes sense to me to draw in the paint event.

This is the part I'm a bit confused about as it was working before and only being triggered on construction or resize.

But the way he made it is more consistent with how we do it today in other classes, so it's only an observation.

To that point, calling update() on construction seems quite redundant, no? It made sense before when the constructor wasn't using paintEvent, but now that it is it should be called naturally as part of the widget constructor. It's not harmful update() can be called just about anywhere, but I think it's redundant. If you look at other classes (e.g. Fader, PixmapButton that override paintEvent(), they do not call update() in the constructor)

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 30, 2017

Member

It's not harmful update() can be called just about anywhere, but I think it's redundant.

I agree, will look into removing it.

Member

Umcaruje commented May 30, 2017

It's not harmful update() can be called just about anywhere, but I think it's redundant.

I agree, will look into removing it.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje
Member

Umcaruje commented May 31, 2017

@tresf done.

@Umcaruje Umcaruje merged commit 53772c0 into LMMS:stable-1.2 May 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017

Allow WidgetTab to use artwork tabs (@StCyr) (#3569)
* First version of artwork tabs for the InstrumentTrackWindow.

This version can only display & manage artwork tabs, which breaks
the InstrumentSoundShapingView as it still uses text tabs.

I'm planing to improve this implementation to let these artwork tabs fall back
to text mode when no artwork is given. This would solve the problem of the
InstrumentSoundShapingView.

* Second version of artwork tabs for the InstrumentTrackWindow.

This version will draw an artwork tab when the TabWidget::addTab function is given
a pixmapName. Otherwise, when pixmapName is NULL, it will fall back drawing a text
tab.

* Created artwork for the artwork tabs.

* 1st PoC for autosizeable artwork tabs.

* TabWidget is 20 pixels tall when it's going to display artwork tabs.

* Added tooltip support for the TabWidget class.

Atm, tooltips are simply tabs' name.

* Imported artworks from RebeccaDeField

* Reverted to 12px tall TabWidget

* Fine tuning for the positioning of artwork tabs: Take into account the caption 'space.

* New artwork for the ENV/LFO tab (has now an ADSR-based look)

* 1) Tabs in TabWidget class have now a "tooltip" attribute. So that they can now show
more meaningfull information then simply the tab's name.
2) Fixed the compilation problem with QT5

* Fine tuning the positioning of highlighted artwork tabs.

* Fixed an issue in TabWidget's artwork tabs autosize function that makes gdb crash
with SIGFPE.

* TabWidget is 17 pixels tall when it's going to display artwork tabs.

* Removed underscore prefix for function parameters as coding convention has changed.

(Request at https://github.com/LMMS/lmms/pull/2599/files/dccf9f411996c8c866ff1086b65f15020ef08cd9#r61165005)

Cyrille

* Removed background gradient for TabWidget as LMMS is going to a more flat design.

Cyrille

* Increased the graphical TabWidget's height by 2 pixels for eye-candy.

The InstrumentTrackWindow's height has been increased by the same amount.

Cyrille

* Removed gradient in GrouBox widgets as LMMS is going for a more flattened design.

Cyrille

* Made the background of TabWidget themeable

Cyrille

* The highlighting color for a TabWidget'selected tab is now themeable.

* Made TabWidget's Title text and tab text themeable.

* Added a darker background to the TabWidget's tab bar.

* Further flatened the design of TabWidget

* Flatened the design of the GroupBox widget

* Fine tuning the placement of TabWidgets' highlighting background
+ some code cleaning in TabWidgets

* Made the TabWidget's title background and borders themeable

* TabWidget - Artwork tabs: Do not change the icon color when it is highlighted

* TabWidget: Made the artworks' color themeable

* Adapted format to follow LMMS coding conventions

* Some more blank spaces to tabs translation to comply with LMMS coding standards.

* Some more blank spaces to tabs translation to comply with LMMS coding standards.

* Revert "TabWidget: Made the artworks' color themeable"

This reverts commit 5b162c0.

Conflicts:
	src/gui/widgets/TabWidget.cpp

Reason: Artwork's color themeability had the side-effect that it removed the artworks' alpha
channel, thus making them ugly.

* Made GroupBox's background color themeable

* Update background color, only use one set of images

* Use name as tooltip, more descriptive names

* Update icons and colors

* more things

* formatting fixes

* Remove update() from constructor

PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017

Allow WidgetTab to use artwork tabs (@StCyr) (#3569)
* First version of artwork tabs for the InstrumentTrackWindow.

This version can only display & manage artwork tabs, which breaks
the InstrumentSoundShapingView as it still uses text tabs.

I'm planing to improve this implementation to let these artwork tabs fall back
to text mode when no artwork is given. This would solve the problem of the
InstrumentSoundShapingView.

* Second version of artwork tabs for the InstrumentTrackWindow.

This version will draw an artwork tab when the TabWidget::addTab function is given
a pixmapName. Otherwise, when pixmapName is NULL, it will fall back drawing a text
tab.

* Created artwork for the artwork tabs.

* 1st PoC for autosizeable artwork tabs.

* TabWidget is 20 pixels tall when it's going to display artwork tabs.

* Added tooltip support for the TabWidget class.

Atm, tooltips are simply tabs' name.

* Imported artworks from RebeccaDeField

* Reverted to 12px tall TabWidget

* Fine tuning for the positioning of artwork tabs: Take into account the caption 'space.

* New artwork for the ENV/LFO tab (has now an ADSR-based look)

* 1) Tabs in TabWidget class have now a "tooltip" attribute. So that they can now show
more meaningfull information then simply the tab's name.
2) Fixed the compilation problem with QT5

* Fine tuning the positioning of highlighted artwork tabs.

* Fixed an issue in TabWidget's artwork tabs autosize function that makes gdb crash
with SIGFPE.

* TabWidget is 17 pixels tall when it's going to display artwork tabs.

* Removed underscore prefix for function parameters as coding convention has changed.

(Request at https://github.com/LMMS/lmms/pull/2599/files/dccf9f411996c8c866ff1086b65f15020ef08cd9#r61165005)

Cyrille

* Removed background gradient for TabWidget as LMMS is going to a more flat design.

Cyrille

* Increased the graphical TabWidget's height by 2 pixels for eye-candy.

The InstrumentTrackWindow's height has been increased by the same amount.

Cyrille

* Removed gradient in GrouBox widgets as LMMS is going for a more flattened design.

Cyrille

* Made the background of TabWidget themeable

Cyrille

* The highlighting color for a TabWidget'selected tab is now themeable.

* Made TabWidget's Title text and tab text themeable.

* Added a darker background to the TabWidget's tab bar.

* Further flatened the design of TabWidget

* Flatened the design of the GroupBox widget

* Fine tuning the placement of TabWidgets' highlighting background
+ some code cleaning in TabWidgets

* Made the TabWidget's title background and borders themeable

* TabWidget - Artwork tabs: Do not change the icon color when it is highlighted

* TabWidget: Made the artworks' color themeable

* Adapted format to follow LMMS coding conventions

* Some more blank spaces to tabs translation to comply with LMMS coding standards.

* Some more blank spaces to tabs translation to comply with LMMS coding standards.

* Revert "TabWidget: Made the artworks' color themeable"

This reverts commit 5b162c0.

Conflicts:
	src/gui/widgets/TabWidget.cpp

Reason: Artwork's color themeability had the side-effect that it removed the artworks' alpha
channel, thus making them ugly.

* Made GroupBox's background color themeable

* Update background color, only use one set of images

* Use name as tooltip, more descriptive names

* Update icons and colors

* more things

* formatting fixes

* Remove update() from constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment