D16839: Force black pen when painting unstyled TextLabel

Sun Dec 2 21:20:16 GMT 2018

asemke added a comment.

  In D16839#365584 <https://phabricator.kde.org/D16839#365584>, @mikhailru wrote:

  > @asemke, thanks for your explanation! I've done some research, and it looks like at least 3 issues exist:
  >
  > (1) The first case is when theme == "". This is what happens when no theme has been configured via the settings dialog and labplot2rc does not contain "Theme" key in [Settings_Worksheet] group, or when labplot2rc does not exist (first run). In this case, CartesianPlot does not apply theme to children, including axes labels:
  >
  >   //in CartesianPlot::childAdded
  >   if (!d->theme.isEmpty()) {
  >           const auto* elem = dynamic_cast<const WorksheetElement*>(child);
  >   	if (elem) {
  >   		KConfig config(ThemeHandler::themeFilePath(d->theme), KConfig::SimpleConfig);
  >   		const_cast<WorksheetElement*>(elem)->loadThemeConfig(config);
  >   	}
  >   } else {
  >   	//no theme is available, apply the default colors for curves only, s.a. XYCurve::loadThemeConfig()
  >
  >
  > This results in d->textWrapper.text in TextLabels containing plain text without any formatting. I've checked XMLs to verify:
  >
  >   <textLabel creation_time="2018-23-11 00:20:41:571" name="x axis 1">
  >   <comment></comment>
  >   <geometry x="-10.5833" y="605.444" horizontalPosition="3" verticalPosition="3" horizontalAlignment="1" verticalAlignment="1" rotationAngle="0" visible="1"/>
  >   <text>x axis 1</text>
  >   <format teXUsed="0" fontFamily="Computer Modern" fontSize="-1" fontPointSize="12" fontWeight="50" fontItalic="0" teXFontColor_r="0" teXFontColor_g="0" teXFontColor_b="0"/>
  >   </textLabel>
  >
  >
  > Compare this to the XML obtained with theme != "":
  >
  >   <textLabel creation_time="2018-23-11 00:31:30:976" name="x axis 1">
  >   <comment />
  >       <geometry x="-10.5833" y="605.444" horizontalPosition="3" verticalPosition="3" horizontalAlignment="1" verticalAlignment="1" rotationAngle="0" visible="1" />
  >       <text>
  >             <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd">
  >             <html><head><meta name="qrichtext" content="1" /><style type="text/css">
  >             p, li { white-space: pre-wrap; }
  >             </style></head><body style=" font-family:'Noto Sans'; font-size:10pt; font-weight:400; font-style:normal;">
  >             <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;"><span style=" color:#ffffff; background-color:#000000;">x axis 1</span></p></body></html>
  >       </text>
  >       <format teXUsed="0" fontFamily="Computer Modern" fontSize="-1" fontPointSize="12" fontWeight="50" fontItalic="0" teXFontColor_r="0" teXFontColor_g="0" teXFontColor_b="0" />
  >   </textLabel>
  >
  >
  > This text without formatting is painted with the default color, taken from the Qt theme. And this is the case which my original patch addressed.

  Ok. This makes sense. But let's use maybe the color QApplication::palette().color(QPalette::Active,QPalette::Text ) ) instead of hard-coded Qt::black.

  > (2) theme == "None". I suppose it's what you were talking about in "On the last screenshot...".  In this case theme != "", but ThemeHandler::themeFilePath cannot find theme file and returns "", so values hardcoded in KConfigGroup::readEntry calls are used. Unfortunately, both [Label]/FontColor (TextLabel.cpp:842) and [Worksheet]/BackgroundFirstColor (Worksheet.cpp:96) use Qt::white as default.

  Thanks for digging into this. I just debugged a bit, too. Having "None" here is a bug. We should save an empty string if "None" was selected. I'll fix this soon.

  > (3) LabelWidget doesn't look nice with some Qt theme/label colors combination.
  > 
  > What can be done about this? I'm not deeply familiar with the codebase, but I have some ideas. If you find them worthwhile, I'll submit patches.
  > 
  > (1) I'm not sure whether using labels text without formatting is a supposed mode of operation. Maybe we should somehow fallback to "None" (hardcoded) theme instead of current behaviour?

  Having text without formatting is perfectly fine. User simply types in some text without the need for additional formatting. Once this bug with "None" is fixed and we have empty string for the theme name, the theme settings with hard-coded default values won't be loaded anymore as you described above. Together with the default Qt theme color for the painter as you originally proposed we should be ok here.

  > (2) Swap background/foreground color defaults in TextLabel::loadThemeConfig. This one looks harmless.

  This shouldn't be needed anymore once we adressed the two points mentioned above.

  > (3) Concerning this one, I can't come up with a solution which doesn't hurt present functionality. I'll think a bit more. Btw, does [Label]/BackgroundColor affect plot appearance in any way?

  In principle it should be possible to change the background color for different parts of the text as shown on this screenshot:
  F6451811: grafik.png <https://phabricator.kde.org/F6451811>
  But this is ignored at the moment and I think this is a bug in QStaticText that we've mentioned somewhere in the code. We need to check this. If this is really a problem with QStaticText, we should remove (or hide for a moment) the widgets for the background color in order to reduce the confusion.

REPOSITORY
  R262 LabPlot

REVISION DETAIL
  https://phabricator.kde.org/D16839

To: mikhailru, #labplot
Cc: asemke, cfeck, kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20181202/17f1c6f3/attachment-0001.html>