Scide/theme mgmt fix #1297

vdonnefort · 2015-01-28T09:36:21Z

Hi,

Following our discussion with @miczac (#1150), here's some theme related fixes.

This patch adds missing elements to the list of keys to import when migrating to the new theme management feature.

Only background color is used for current line. See this previous patch: ide: themes: disable unused options display for currentLine

Since the transparent color can't be stored, this patch makes sure that when the picked brush is Qt::NoBrush (~ color transparent), the foreground is not set. It fixes the transparent color for foreground.

Since the theme class is not a Q_OBJECT, there is no need to put it into ide_moc_hdr. This patch fixes the following warning: sc-ide/core/settings/theme.hpp:0: Note: No relevant classes found

vdonnefort · 2015-03-03T09:45:53Z

Hi,

I just added a patch which fixes the warning "theme.hpp:0: Note: No relevant classes found. No output generated."

miczac · 2015-03-14T17:01:06Z

Hi, would be lovely to have this commits pulled! ;) thanks!

vdonnefort · 2015-03-24T08:41:10Z

@telephon @jleben @muellmusik This PR is needed for 3.7. Can you label it?

bagong · 2015-03-24T10:35:48Z

Or just merge and let testers find problems? In reality pull requests hardly get tested. This one (well, the one this builds upon) had more testing and discussion than most others. I have been using this a bit, seems to work fine. The one confusing thing I found is that you have to create a new theme, before you can change certain settings, but I guess that's by design? (try e.g. to change background color of "matching brackets" in the default settings - currently a hot issue :) )

scztt · 2015-03-26T05:48:49Z

👍 to merge

bagong · 2015-03-26T09:06:00Z

Sorry, I found two more issues, one very small, the other one significant, but not necessarily directly related to your PR:

disabling the foreground button for 'current line' seems okay, although a bit normative. But then the color box (and the display example) should actually display the foreground color chosen for default text. That is currently not the case, both box and sample always display black.
the color picker (pipette) doesn't work. If you click it, nothing happens. After you close the preferences dialogue though, the cursor displays as magnifying glass in the editor (this is OS X 10.10.2/Qt5.4.1). Obviously the color picker was waiting for the dialogue to close :)

vdonnefort · 2015-03-26T09:21:02Z

@bagong:

The behavior you're encountering is perfectly normal: the "current line" is not used and so is disabled: vdonnefort@83e8490

bagong · 2015-03-26T09:32:22Z

Yes, that's not my point.

Any comments on the color picker?

vdonnefort · 2015-03-26T09:40:54Z

@bagong @scztt @miczac Do you agree if instead of disabled foreground options I hide them?

bagong · 2015-03-26T10:03:08Z

Well, I don't want to be too picky, so let somebody else make a judgement. I'll just say that in that case the text sample to the left will still display black as foreground color, regardless of the actual default Text color, so the user will still be confused. But there certainly are more pressing issues. So from my side up to your own judgement.
Would you please say if the color-picker thing is connected to your contribution? If not, I will file a general issue.

vdonnefort · 2015-03-26T10:07:17Z

@bagong, when I make this contribution I've been asked for disabling these unused options. But the fact that the foreground is not used is not related to this PR.

bagong · 2015-03-26T10:57:07Z

Let me try to be clearer: There are two issues.

GUI-feedback for the user to see if she likes her choices. She can chose fore- and background for default text, see the colors in the boxes, and see the resulting text-background-combination with a sample in the large selection box. This doesn't work in the case of "Current line" where as foreground color always black is displayed, regardless of the color chosen for default "Text". This could make the user think, the "Current Line" can only display black as foreground color. Try to imagine somebody who has no idea of the implementation.
The color-picker pipette issue: if you click on the color-box for fore- or background, the the system color picker will open. At the left bottom it displays a color box that shows the color you have chosen. To it's right there is a icon resembling a pipette. If you click it, you should be able to pick up a color from the graphical environment. The cursor will change into a magnifying glass to enhance the precision of your color-"pick". This is quite useful, but it doesn't work in the context of the preferences dialog, the functionality is dead. Instead you get the cursor shaped as magnifying glass when you close the preferences dialog. This is a proper and reproducible bug - but it might not be related directly to your pull-request. I am asking for a comment on exactly that question.

I am sorry, I won't be able to comment further on this, have to run...

bagong · 2015-04-03T13:10:43Z

So I took the time to check in vanilla master: both issues are there as well. I'll file two issues and think this should be merged.

telephon · 2015-04-03T20:45:56Z

@bagong So we should merge and then treat these issues separately?

bagong · 2015-04-03T20:46:37Z

Yes, I've already filed issues.

crucialfelix · 2016-03-17T15:58:58Z

let's merge now, confirm ?

vdonnefort · 2016-03-17T16:29:03Z

Yes

Fix SCIDE theme management

crucialfelix · 2016-03-17T16:30:18Z

Damn we need some structure around here. Over a year to merge a PR ? I just merged a shit tonne of pull requests.

vdonnefort · 2016-03-17T16:34:32Z

And I'm thankful for that, these are bugfix and should have been merged on time for 3.7.

crucialfelix · 2016-03-17T16:37:11Z

There was such an incredible backlog it was hard for me to go through so
many issues and PRs.
3.7 was more about shipping what was already done and merged.
3.8 comes June 1

On Thu, Mar 17, 2016 at 5:34 PM Vincent Donnefort notifications@github.com
wrote:

And I'm thankful for that, these are bugfix and should have been merged on
time for 3.7.

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#1297 (comment)

https://twitter.com/crucialfelix
https://medium.com/@crucialfelix
http://www.mattermind.com/

vdonnefort added 3 commits January 26, 2015 10:08

scide: theme: add missing keys to legacy import

ee832c9

This patch adds missing elements to the list of keys to import when migrating to the new theme management feature.

scide: theme: disable fg clear button for currentLine

83e8490

Only background color is used for current line. See this previous patch: ide: themes: disable unused options display for currentLine

scide: theme: do not set foreground when the color is transparent

acec435

Since the transparent color can't be stored, this patch makes sure that when the picked brush is Qt::NoBrush (~ color transparent), the foreground is not set. It fixes the transparent color for foreground.

vdonnefort mentioned this pull request Feb 2, 2015

Scide/line number #1302

Merged

scide: remove theme.hpp from ide_moc_hdr

40a45f5

Since the theme class is not a Q_OBJECT, there is no need to put it into ide_moc_hdr. This patch fixes the following warning: sc-ide/core/settings/theme.hpp:0: Note: No relevant classes found

scztt added this to the 3.7 milestone Mar 26, 2015

crucialfelix modified the milestones: 3.8, 3.7.x Feb 29, 2016

crucialfelix added the waiting for consensus label Mar 17, 2016

crucialfelix added a commit that referenced this pull request Mar 17, 2016

Merge pull request #1297 from vdonnefort/scide/theme-mgmt-fix

04ea7e5

Fix SCIDE theme management

crucialfelix merged commit 04ea7e5 into supercollider:master Mar 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scide/theme mgmt fix #1297

Scide/theme mgmt fix #1297

vdonnefort commented Jan 28, 2015

vdonnefort commented Mar 3, 2015

miczac commented Mar 14, 2015

vdonnefort commented Mar 24, 2015

bagong commented Mar 24, 2015

scztt commented Mar 26, 2015

bagong commented Mar 26, 2015

vdonnefort commented Mar 26, 2015

bagong commented Mar 26, 2015

vdonnefort commented Mar 26, 2015

bagong commented Mar 26, 2015

vdonnefort commented Mar 26, 2015

bagong commented Mar 26, 2015

bagong commented Apr 3, 2015

telephon commented Apr 3, 2015

bagong commented Apr 3, 2015

crucialfelix commented Mar 17, 2016

vdonnefort commented Mar 17, 2016

crucialfelix commented Mar 17, 2016

vdonnefort commented Mar 17, 2016

crucialfelix commented Mar 17, 2016

Scide/theme mgmt fix #1297

Scide/theme mgmt fix #1297

Conversation

vdonnefort commented Jan 28, 2015

vdonnefort commented Mar 3, 2015

miczac commented Mar 14, 2015

vdonnefort commented Mar 24, 2015

bagong commented Mar 24, 2015

scztt commented Mar 26, 2015

bagong commented Mar 26, 2015

vdonnefort commented Mar 26, 2015

bagong commented Mar 26, 2015

vdonnefort commented Mar 26, 2015

bagong commented Mar 26, 2015

vdonnefort commented Mar 26, 2015

bagong commented Mar 26, 2015

bagong commented Apr 3, 2015

telephon commented Apr 3, 2015

bagong commented Apr 3, 2015

crucialfelix commented Mar 17, 2016

vdonnefort commented Mar 17, 2016

crucialfelix commented Mar 17, 2016

vdonnefort commented Mar 17, 2016

crucialfelix commented Mar 17, 2016