WIP: KWeather XDG icon names and icon theme support #18

Open
blu.256 wants to merge 27 commits from feat/kweather-std-icons into master
blu.256 commented 2 months ago
Collaborator

This PR:

  • Renames icons to (mostly) standard icon names for compatibility with other themes,
  • Updates code in weatherlib to correctly handle new icon names,
  • Adds support for weather icons from icon theme selected by user.

Comments/suggestions/improvements are, of course, welcome.

Tested with Oxygen, Masalla, KFaenza.

Contains minor internal API changes which don't seem to affect anything other than intended.

Commits will be squashed prior to merge.

This PR: * Renames icons to (mostly) standard icon names for compatibility with other themes, * Updates code in weatherlib to correctly handle new icon names, * Adds support for weather icons from icon theme selected by user. Comments/suggestions/improvements are, of course, welcome. Tested with Oxygen, Masalla, KFaenza. Contains minor internal API changes which don't seem to affect anything other than intended. Commits will be squashed prior to merge.
blu.256 added this to the R14.1.0 release milestone 2 months ago
blu.256 added the
PR/rfc
label 2 months ago
blu.256 added 5 commits 2 months ago
45a0e6333c
Renamed KWeather icons to adhere to icon theme standards.
2ae9b7aa18
KWeather: use glob to find and install weather icons
540787f294
KWeather: updated icon handling.
02dd633f68
KWeather: updated icon name in DockWidget class
dd2546046e
KWeather: support for weather icons from icon theme.
blu.256 added 1 commit 2 months ago
blu.256 requested review from SlavekB 2 months ago
blu.256 requested review from MicheleC 2 months ago
Owner

The code looks good at a quick look through. I gave it a go, build and installed. To my surprise, I ended up with missing icons in both the applet and the konqueror sidebar.
All new icons are correctly installed in /opt/trinity/share/apps/kweather.
My icon theme is a custom one (Masalla), but changing to crystalSVG also dispaly the same issue.
I even restarted my system just in case.

The code looks good at a quick look through. I gave it a go, build and installed. To my surprise, I ended up with missing icons in both the applet and the konqueror sidebar. All new icons are correctly installed in /opt/trinity/share/apps/kweather. My icon theme is a custom one (Masalla), but changing to crystalSVG also dispaly the same issue. I even restarted my system just in case.
Poster
Collaborator

Hello, could you please report the debugging output of kweatherservice? It seemed to work on my machine. I'll be testing this on a clean VM later in the day, but your input coild prove valuable.

Hello, could you please report the debugging output of `kweatherservice`? It seemed to work on my machine. I'll be testing this on a clean VM later in the day, but your input coild prove valuable.
Owner

Hello, could you please report the debugging output of kweatherservice? It seemed to work on my machine. I'll be testing this on a clean VM later in the day, but your input coild prove valuable.

Hi Philippe,
I am not sure I understand exactly what you need. Is it the log that can be enabled from KWeather applet config? If so, here is a short extract:

Date,Last Updated,Wind Speed & Direction,Temperature,Pressure,Cover,Visibility,Current Weather
Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km,  ;  Haze
Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km,  ;  Haze
Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km,  ;  Haze
Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km,  ;  Haze

If not, please let me know where to find what you need 😄

> Hello, could you please report the debugging output of `kweatherservice`? It seemed to work on my machine. I'll be testing this on a clean VM later in the day, but your input coild prove valuable. Hi Philippe, I am not sure I understand exactly what you need. Is it the log that can be enabled from KWeather applet config? If so, here is a short extract: ``` Date,Last Updated,Wind Speed & Direction,Temperature,Pressure,Cover,Visibility,Current Weather Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km, ; Haze Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km, ; Haze Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km, ; Haze Sunday 14 November 2021 17:31,Sunday 14 November 2021 16:58,9 km/h WNW,14.2°C,1017 hPa,Scattered clouds at 1981 meters,9.6km, ; Haze ``` If not, please let me know where to find what you need :smile:
Poster
Collaborator

Tested on another machine, it seems broken here too except for one icon theme. I'll be looking into this regression.

Is it the log that can be enabled from KWeather applet config?

No, I meant the debugging output of kdDebug()'s, which you can set via tdedebugdialog --fullmode to print either to stdout or to a file. In this case, the program ID is 12006. I'm interested in lines such as the following:

[2021/11/14 17:42:23.039] [kweatherservice] Get the current weather icon..
[2021/11/14 17:42:23.041] [kweatherservice] icon: /usr/share/icons/oxygen/base/16x16/status/weather-clouds-night.png
[2021/11/14 17:42:23.042] [kweatherservice] Get the current weather icon..
[2021/11/14 17:42:23.042] [kweatherservice] icon: /usr/share/icons/oxygen/base/16x16/status/weather-clouds-night.png
Tested on another machine, it seems broken here too except for one icon theme. I'll be looking into this regression. > Is it the log that can be enabled from KWeather applet config? No, I meant the debugging output of `kdDebug()`'s, which you can set via `tdedebugdialog --fullmode` to print either to stdout or to a file. In this case, the program ID is 12006. I'm interested in lines such as the following: ``` [2021/11/14 17:42:23.039] [kweatherservice] Get the current weather icon.. [2021/11/14 17:42:23.041] [kweatherservice] icon: /usr/share/icons/oxygen/base/16x16/status/weather-clouds-night.png [2021/11/14 17:42:23.042] [kweatherservice] Get the current weather icon.. [2021/11/14 17:42:23.042] [kweatherservice] icon: /usr/share/icons/oxygen/base/16x16/status/weather-clouds-night.png ```
blu.256 added 1 commit 2 months ago
156b6b597b
KWeather: Fixed fallback to default icons
SlavekB requested changes 2 months ago
SlavekB left a comment

There seem to be some mistakes that need to be fixed.
See comments below.

else if ( weatherInfo.clouds > 8 && weatherInfo.clouds < 63)
weatherInfo.theWeather = iconName( "cloudy4" );
weatherInfo.theWeather = iconName( WeatherIcon::Cloudy, night, 4 );
else if (weatherInfo.clouds < 0)
Poster
Owner

I believe that the condition for weatherInfo.clouds < 0 would be clearer to be placed first, because all other conditions relate to higher and higher clouds.

I believe that the condition for `weatherInfo.clouds < 0` would be clearer to be placed first, because all other conditions relate to higher and higher clouds.
Poster
Collaborator

Now that I'm looking through the rest of metar_parser.cpp, there isn't any possibility of this being < 0. So maybe this case should really be dismissed.

Now that I'm looking through the rest of `metar_parser.cpp`, there isn't any possibility of this being < 0. So maybe this case should really be dismissed.
SlavekB marked this conversation as resolved
weatherInfo.clouds = 30;
if (weatherInfo.clouds >= 0 && weatherInfo.clouds <= 10)
Poster
Owner

If there may be situations where WeatherInfo.clouds < 0, similar condition should probably be also here.

If there may be situations where `WeatherInfo.clouds < 0`, similar condition should probably be also here.
SlavekB marked this conversation as resolved
weatherInfo.clouds = 30;
if (weatherInfo.clouds >= 0 && weatherInfo.clouds <= 10)
Poster
Owner

If there may be situations where WeatherInfo.clouds < 0, similar condition should probably be also here.

If there may be situations where `WeatherInfo.clouds < 0`, similar condition should probably be also here.
SlavekB marked this conversation as resolved
weatherInfo.clouds = 30;
if (weatherInfo.clouds >= 0 && weatherInfo.clouds <= 8)
Poster
Owner

If there may be situations where WeatherInfo.clouds < 0, similar condition should probably be also here.

If there may be situations where `WeatherInfo.clouds < 0`, similar condition should probably be also here.
SlavekB marked this conversation as resolved
TQString _iconName;
if( strength != 0 )
{
// Simple
Poster
Owner

The comment indicates Simple, but the call corresponds to Ranged.

The comment indicates `Simple`, but the call corresponds to `Ranged`.
blu.256 marked this conversation as resolved
}
else
{
// Ranged
Poster
Owner

The comment indicates Ranged, but the call corresponds to Simple.

The comment indicates `Ranged`, but the call corresponds to `Simple`.
blu.256 marked this conversation as resolved
case Fog:
{
name = "weather-fog";
if( night && iconExists( TQString(name.latin1()).append("-night")) )
Poster
Owner

Can there be a risk that .latin1() causes unwanted degradation of some characters?
There may be more appropriate to use .copy()?

Can there be a risk that `.latin1()` causes unwanted degradation of some characters? There may be more appropriate to use `.copy()`?
blu.256 marked this conversation as resolved
case Mist:
{
name = "weather-mist";
if( night && iconExists( TQString(name.latin1()).append("-night")) )
Poster
Owner

Same as above.

Same as above.
blu.256 marked this conversation as resolved
break;
}
case 5: { iconName = "weather-many-clouds"; return; }
Poster
Owner

Here, it is likely to be name = instead of iconName = because the iconName value is overwritten by a few lines below.

Here, it is likely to be `name =` instead of `iconName =` because the `iconName` value is overwritten by a few lines below.
Poster
Collaborator

If you notice, wherever I use iconName instead of name I also use return instead of break. This is done in cases where there is no need for further execution (e.g. to append "-night" or "-day") so I don't really see any problems with the logic.

If you notice, wherever I use `iconName` instead of `name` I also use `return` instead of `break`. This is done in cases where there is no need for further execution (e.g. to append "-night" or "-day") so I don't really see any problems with the logic.
Poster
Owner

Thank you for the notice – I overlooked there was used return.

Thank you for the notice – I overlooked there was used `return`.
SlavekB marked this conversation as resolved
}
case 5: { iconName = "weather-many-clouds"; return; }
default: { iconName = "weather-clouds"; return; }
Poster
Owner

Same as above.

Same as above.
SlavekB marked this conversation as resolved
case 1: { name = "weather-showers-scattered"; break; }
case 2: { name = "weather-showers"; break; }
case 3:
default: { iconName = "weather-showers"; return; }
Poster
Owner

Here, it is likely to be name = instead of iconName = because the iconName value is overwritten by a few lines below.

Here, it is likely to be `name =` instead of `iconName =` because the iconName value is overwritten by a few lines below.
SlavekB marked this conversation as resolved
case 2:
{
name = "weather-snow-moderate";
if (! iconExists( TQString(name.latin1()).append("-day")) )
Poster
Owner

Can there be a risk that .latin1() causes unwanted degradation of some characters?
There may be more appropriate to use .copy()?

Can there be a risk that `.latin1() `causes unwanted degradation of some characters? There may be more appropriate to use `.copy()`?
blu.256 marked this conversation as resolved
case 3:
{
name = "weather-snow-ample";
if ( iconExists( TQString(name.latin1()).append("-day") ) )
Poster
Owner

Can there be a risk that .latin1() causes unwanted degradation of some characters?
There may be more appropriate to use .copy()?

Can there be a risk that `.latin1()` causes unwanted degradation of some characters? There may be more appropriate to use `.copy()`?
blu.256 marked this conversation as resolved
{
name = "weather-snow-ample";
if ( iconExists( TQString(name.latin1()).append("-day") ) )
break;
Poster
Owner

This use break especially without block {...} looks very confusing. At first glance it seems that here it was forgotten write a block for if. Only at the second look shows that execution will continue case 4:.

This use `break` especially without block `{...}` looks very confusing. At first glance it seems that here it was forgotten write a block for `if`. Only at the second look shows that execution will continue `case 4:`.
Poster
Collaborator

I put it in a brackets block as suggested. Does it look clearer?

I put it in a brackets block as suggested. Does it look clearer?
blu.256 marked this conversation as resolved
if ( iconExists( TQString(name.latin1()).append("-day") ) )
break;
}
case 4: { iconName = "weather-snow-scattered"; return; }
Poster
Owner

Here, it is likely to be name = instead of iconName = because the iconName value is overwritten by a few lines below.

Here, it is likely to be `name =` instead of `iconName =` because the `iconName` value is overwritten by a few lines below.
SlavekB marked this conversation as resolved
}
case 4: { iconName = "weather-snow-scattered"; return; }
case 5:
default: { iconName = "weather-snow"; return; }
Poster
Owner

Same as above.

Same as above.
SlavekB marked this conversation as resolved
case 2:
{
name = "weather-storm-moderate";
if (! iconExists( TQString(name.latin1()).append("-day")) )
Poster
Owner

Can there be a risk that .latin1() causes unwanted degradation of some characters?
There may be more appropriate to use .copy()?

Can there be a risk that `.latin1()` causes unwanted degradation of some characters? There may be more appropriate to use `.copy()`?
blu.256 marked this conversation as resolved
break;
}
case 3:
default: { iconName = "weather-storm"; return; }
Poster
Owner

Here, it is likely to be name = instead of iconName = because the iconName value is overwritten by a few lines below.

Here, it is likely to be `name =` instead of `iconName =` because the `iconName` value is overwritten by a few lines below.
SlavekB marked this conversation as resolved
blu.256 changed title from feat/kweather-std-icons to KWeather standard icon names 2 months ago
blu.256 changed title from KWeather standard icon names to WIP: KWeather standard icon names 2 months ago
Owner

I tested the current code, I see improvement but still not fully ok. "Melbourne Airport" shows no icon for example. I have attached the debug log as you requested.

I tested the current code, I see improvement but still not fully ok. "Melbourne Airport" shows no icon for example. I have attached the debug log as you requested.
blu.256 added 3 commits 2 months ago
77aa6cfa2a
KWeather: Better icon name string handling.
96d733b426
KWeather: Fixed comments
767db478fd
KWeather: Use brackets block for better readability
blu.256 added 1 commit 2 months ago
0dcd5cc1ae
KWeather: remove clouds < 0 case.
blu.256 requested review from SlavekB 2 months ago
blu.256 added 1 commit 2 months ago
d8a52e2665
KWeather: improved fallback mechanism.
Poster
Collaborator

The new fallback code is ready for testing. It is now also (IMO) much cleaner an implementation, is easily extendable and provides enough alternatives/closest matches for every case, plus there sould be no fall-throughs in the new code.

The new fallback code is ready for testing. It is now also (IMO) much cleaner an implementation, is easily extendable and provides enough alternatives/closest matches for every case, plus there sould be no fall-throughs in the new code.
Poster
Collaborator

Another problem: KWeather does not display scalable (SVG) icons.

This is also probably related to your problem, @MicheleC:

icon: /usr/share/icons/masalla/status/16x16/weather-clouds.svg

Every SVG icon theme I tested with seems to have the problem (Breeze, Masalla, Haiku). Oxygen, for example, is based on raster (PNG) icons, so no problem is manifested there.

Another problem: KWeather does not display scalable (SVG) icons. This is also probably related to your problem, @MicheleC: ``` icon: /usr/share/icons/masalla/status/16x16/weather-clouds.svg ``` Every SVG icon theme I tested with seems to have the problem (Breeze, Masalla, Haiku). Oxygen, for example, is based on raster (PNG) icons, so no problem is manifested there.
Owner

I tested the latest code and now I have no icons at all, nor in the weather applet nor in the konqueror weather bar.

I tested the latest code and now I have no icons at all, nor in the weather applet nor in the konqueror weather bar.
blu.256 added 1 commit 2 months ago
0d1d4c5bfc
KWeather: Use TDEIconLoader with absolute path to load icons.
blu.256 added 1 commit 2 months ago
56d015533a
KWeather: Added fallbacks to "weather-snow"
blu.256 changed title from WIP: KWeather standard icon names to KWeather standard icon names 2 months ago
blu.256 changed title from KWeather standard icon names to KWeather XDG icon names and icon theme support 2 months ago
SlavekB approved these changes 2 months ago
SlavekB left a comment

The final way of choosing the icons I like. I made a test on my system with a classic icon set (built-in icons in kweather are used) and everything seems to be okay.

Owner

I have done a test and PR seems to work well now with various icon themes.
Well done!

I would like to raise another topic related to this PR.
Masalla weather icons are color-less, all plain gray. With the original code, I could get the "standard" kweather icons, who have plenty of colors.
I think it would be good to have a checkbox option to let the user choose between "classic icons" and "theme based icons", so people used to the original icons can keep using them if they prefer. What do you think?
I don't really use KWeather, so it doesn't matter to me. But the point I am trying to make is that people who have used that for years may be used to some specific icons for the weather and would be better to provide them with the aforementioned choice.

I have done a test and PR seems to work well now with various icon themes. Well done! I would like to raise another topic related to this PR. Masalla weather icons are color-less, all plain gray. With the original code, I could get the "standard" kweather icons, who have plenty of colors. I think it would be good to have a checkbox option to let the user choose between "classic icons" and "theme based icons", so people used to the original icons can keep using them if they prefer. What do you think? I don't really use KWeather, so it doesn't matter to me. But the point I am trying to make is that people who have used that for years may be used to some specific icons for the weather and would be better to provide them with the aforementioned choice.
Owner

Yes, add a checkbox to allow to prefer KWeather custom icon theme instead of the system icon theme sounds like a good idea. This is similar to the checkbox Use custom icon theme in Amarok.

Yes, add a checkbox to allow to prefer KWeather custom icon theme instead of the system icon theme sounds like a good idea. This is similar to the checkbox Use custom icon theme in Amarok.
Poster
Collaborator

Souds like a good idea.

Souds like a good idea.
Owner

Great, we are all aligned here. Should default be old behavior (original icons) or new behavior (system icons)?

Great, we are all aligned here. Should default be old behavior (original icons) or new behavior (system icons)?
Owner

I think that the system icon theme should be the default.

I think that the system icon theme should be the default.
Poster
Collaborator

@MicheleC

Great, we are all aligned here. Should default be old behavior (original icons) or new behavior (system icons)?

I'm in favour of the new behaviour since:

a) The default icon theme is Crystal, which itself has no weather icons. Thus, the KWeather icons woild be shown by default anyway.

b) It would be nice of KWeather to follow user's icon theme preference by default.

@MicheleC >Great, we are all aligned here. Should default be old behavior (original icons) or new behavior (system icons)? I'm in favour of the new behaviour since: a) The default icon theme is Crystal, which itself has no weather icons. Thus, the KWeather icons woild be shown by default anyway. b) It would be nice of KWeather to follow user's icon theme preference by default.
Owner

Sounds good, let's go for it since we all agree again 😄

Sounds good, let's go for it since we all agree again :smile:
blu.256 added 1 commit 1 month ago
831440b7b3
Implemented icon theme option.
Poster
Collaborator

I finally got round to finalizing the icon theme option. A little testing might be required. I tested in on the panel applet and so far it seems okay.

Also, is this PR to be part of R14.1.0 or the next release?

I finally got round to finalizing the icon theme option. A little testing might be required. I tested in on the panel applet and so far it seems okay. Also, is this PR to be part of R14.1.0 or the next release?
SlavekB requested changes 1 month ago
SlavekB left a comment

There are several little things for consideration and change.

}
TQString MetarParser::iconName( const TQString &icon ) const
void MetarParser::getIcon( int condition, bool night, int strength )
Poster
Owner

The name of the method seems very confusing to me. Although it contains get in the name, it does not return anything as a return value, nor through a passed argument. In fact, it actually sets the icon.

Please try to find some more accurate name of this method.

The name of the method seems very confusing to me. Although it contains `get` in the name, it does not return anything as a return value, nor through a passed argument. In fact, it actually sets the icon. Please try to find some more accurate name of this method.
Poster
Collaborator

Maybe saveIconNamePath(...) would be more fitting?

Maybe `saveIconNamePath(...)` would be more fitting?
Poster
Owner

Yes, it seems more fitting. Thank you.

Yes, it seems more fitting. Thank you.
blu.256 marked this conversation as resolved
</property>
<property name="text">
<string>&amp;Location:</string>
<string>Lo&amp;cation:</string>
Poster
Owner

Is the accelerator change needed?
This will invalidate existing translations.

Is the accelerator change needed? This will invalidate existing translations.
Poster
Collaborator

I don't remember changing existing accelerators, will fix all these manually.

I don't remember changing existing accelerators, will fix all these manually.
blu.256 marked this conversation as resolved
</property>
<property name="text">
<string>&amp;Show icon only</string>
<string>Show icon onl&amp;y</string>
Poster
Owner

Same as above.

Same as above.
blu.256 marked this conversation as resolved
<string>Show icon onl&amp;y</string>
</property>
<property name="accel">
<string>Alt+Y</string>
Poster
Owner

Are the properties accel needed? This does not make a mess with accelerators if they are changed in translation of the respective string above?

Are the properties `accel` needed? This does not make a mess with accelerators if they are changed in translation of the respective string above?
blu.256 marked this conversation as resolved
<string>Show &amp;icon and temperature</string>
</property>
<property name="accel">
<string>Alt+I</string>
Poster
Owner

Same as above.

Same as above.
blu.256 marked this conversation as resolved
</property>
<property name="text">
<string>Show icon, temperature, &amp;wind and pressure information</string>
<string>Show icon, temperature, wind &amp;and pressure information</string>
Poster
Owner

Same as above.

Same as above.
blu.256 marked this conversation as resolved
<string>&amp;Use system icon theme</string>
</property>
<property name="accel">
<string>Alt+U</string>
Poster
Owner

Same as above.

Same as above.
blu.256 marked this conversation as resolved
<string>Use &amp;KWeather theme</string>
</property>
<property name="accel">
<string>Alt+K</string>
Poster
Owner

Same as above.

Same as above.
blu.256 marked this conversation as resolved
<includehint>kurlrequester.h</includehint>
<includehint>klineedit.h</includehint>
<includehint>kpushbutton.h</includehint>
<includehint>kcolorbutton.h</includehint>
Poster
Owner

Please, you can all

<includehints>
<includehint>...</includehint>
...
</includeints>

change to

<includes>
<include...>...</include>
...
</includes>

?

This prevents potential FTBFS.

Please, you can all ``` <includehints> <includehint>...</includehint> ... </includeints> ``` change to ``` <includes> <include...>...</include> ... </includes> ``` ? This prevents potential FTBFS.
Poster
Collaborator

Good note.

Good note.
blu.256 marked this conversation as resolved
blu.256 added 1 commit 1 month ago
ba77b4d5fb
Preference dialog fixes.
blu.256 added 1 commit 1 month ago
c070e27dc7
Rename getIcon(...) to saveIconNamePath(...)
Owner

I ran a test on my computer. There are a couple of points that need to be looked at.

  1. being able to use system icons and kweather icons is nice. But we can only change them from the "diplay" tab of the config page of the KWeather applet. I could not find a way to change that from Konqueror weather bar.
    I think we need to have a new "Weather Config" tab and move the options for "Logging options" and "Weather icons" from "display" tab to such new tab. Then when we open the config page from the Konqueror weather bar, we bring up both "Weather service" and the new "Weather config" tabs, while from KWeather applet we bring up those two and the additional "display" tab.
    Maybe we can also rename the "Weather service" tab to "Weather stations", so in the end we will have:
    a. Display (only if called from KWeather applet)
    b. Weather Config
    c. Weather Stations

  2. when changing from system icons to kweather icons (or viceversa), only the icon of the station displayed in the KWeather applet is updated. If you have more stations configured, all other icons are not updated. You can check this in the "Weather service" page. It is necessary to restart the weather service for all those icons to be updated. Correct behavior would be to update all the configured station icons when icon style is changed

I ran a test on my computer. There are a couple of points that need to be looked at. 1. being able to use system icons and kweather icons is nice. But we can only change them from the "diplay" tab of the config page of the KWeather applet. I could not find a way to change that from Konqueror weather bar. I think we need to have a new "Weather Config" tab and move the options for "Logging options" and "Weather icons" from "display" tab to such new tab. Then when we open the config page from the Konqueror weather bar, we bring up both "Weather service" and the new "Weather config" tabs, while from KWeather applet we bring up those two and the additional "display" tab. Maybe we can also rename the "Weather service" tab to "Weather stations", so in the end we will have: a. Display (only if called from KWeather applet) b. Weather Config c. Weather Stations 2. when changing from system icons to kweather icons (or viceversa), only the icon of the station displayed in the KWeather applet is updated. If you have more stations configured, all other icons are not updated. You can check this in the "Weather service" page. It is necessary to restart the weather service for all those icons to be updated. Correct behavior would be to update all the configured station icons when icon style is changed
MicheleC reviewed 1 month ago
// Check in theme
for ( TQStringList::Iterator icon = fallback.begin(); icon != fallback.end(); ++icon )
{
kdDebug() << "[WeatherIcon::findIcon] Searching for `" << *icon << "` in theme" << endl;
Poster
Owner

kweather uses 12004 and kweatherservice 12006 as debug ids. We should change all kdDebug calls accordingly.

kweather uses 12004 and kweatherservice 12006 as debug ids. We should change all kdDebug calls accordingly.
blu.256 marked this conversation as resolved
MicheleC reviewed 1 month ago
TQString iPath = iconPath(*icon, true);
if( !( iPath.isNull() ) )
{
kdDebug() << "[WeatherIcon::findIcon] FOUND `" << *icon << "` in theme: " << iPath << endl;
Poster
Owner

kdDebug() already adds a [kweather] at the beginning of the log string (I didn't test, so the exact string may be slightly different). Perhaps we should consider simplify "[WeatherIcon::findIcon]" to something like "[findIcon]" or "(findIcon)" or similar? Not mandatory to do it, just proposing an idea. Feel free to ignore this if you prefer.

kdDebug() already adds a [kweather] at the beginning of the log string (I didn't test, so the exact string may be slightly different). Perhaps we should consider simplify "[WeatherIcon::findIcon]" to something like "[findIcon]" or "(findIcon)" or similar? Not mandatory to do it, just proposing an idea. Feel free to ignore this if you prefer.
blu.256 marked this conversation as resolved
MicheleC reviewed 1 month ago
<cstring>m_kweatherIcons</cstring>
</property>
<property name="text">
<string>Use &amp;KWeather theme</string>
Poster
Owner

How about adding a "classic" here?

string>Use classic &amp;KWeather theme</string>
How about adding a "classic" here? ``` string>Use classic &amp;KWeather theme</string> ```
blu.256 marked this conversation as resolved
MicheleC reviewed 1 month ago
<cstring>m_systemIcons</cstring>
</property>
<property name="text">
<string>&amp;Use system icon theme</string>
Poster
Owner

No "icon" for consistency with next entry. Also this is the weather icon category, so it clear we talk about icons,

<string>&amp;Use system theme</string>
No "icon" for consistency with next entry. Also this is the weather icon category, so it clear we talk about icons, ``` <string>&amp;Use system theme</string> ```
blu.256 marked this conversation as resolved
MicheleC reviewed 1 month ago
<cstring>m_iconTheme</cstring>
</property>
<property name="title">
<string>Weather Icon</string>
Poster
Owner
<string>Weather Icons</string>
``` <string>Weather Icons</string> ```
blu.256 marked this conversation as resolved
Owner

Overall good work! Nice to offer the users the ability to use different icons for KWeather!

Overall good work! Nice to offer the users the ability to use different icons for KWeather!
MicheleC requested changes 1 month ago
MicheleC left a comment

See comments on conversation chat

blu.256 added 2 commits 1 month ago
ded4825e08
Refine kdDebug() calls according to recommendations.
6aff0b474f
prefdialogfdata.ui: wording changes
blu.256 added 2 commits 1 month ago
61e3bdb76a
WeatherService: Added forceUpdateAll() function.
3d20f6aadb
Force all stations to update.
Poster
Collaborator

(in reply to on this comment)

Issue 2 resolved with latest two commits.

I agree with issue 1. One question so far:

a. Display (only if called from KWeather applet)

If this tab is only called from the KWeather applet, wouldn't it be better to call it "Applet"?

(in reply to on [this comment](https://mirror.git.trinitydesktop.org/gitea/TDE/tdetoys/pulls/18#issuecomment-16872)) Issue 2 resolved with latest two commits. I agree with issue 1. One question so far: > a. Display (only if called from KWeather applet) If this tab is only called from the KWeather applet, wouldn't it be better to call it "Applet"?
Owner

(in reply to on this comment)

Issue 2 resolved with latest two commits.

Ok great. I will test again when also issue 1 is fixed.

I agree with issue 1. One question so far:

a. Display (only if called from KWeather applet)

If this tab is only called from the KWeather applet, wouldn't it be better to call it "Applet"?

Maybe "Applet config"? For example:
a. Applet Config
b. Service Config
c. Weather Stations
What do you think?

> (in reply to on [this comment](https://mirror.git.trinitydesktop.org/gitea/TDE/tdetoys/pulls/18#issuecomment-16872)) > > Issue 2 resolved with latest two commits. Ok great. I will test again when also issue 1 is fixed. > I agree with issue 1. One question so far: > > > a. Display (only if called from KWeather applet) > > If this tab is only called from the KWeather applet, wouldn't it be better to call it "Applet"? Maybe "Applet config"? For example: a. Applet Config b. Service Config c. Weather Stations What do you think?
Poster
Collaborator

a. Applet Config
b. Service Config

Maybe naming sections "* Config" in a configuration dialog is a little superfluous? What about:

a. Applet
b. (Weather) Service
c. Stations

> a. Applet Config > b. Service Config Maybe naming sections "* Config" in a configuration dialog is a little superfluous? What about: > a. Applet > b. (Weather) Service > c. Stations
blu.256 added 1 commit 4 weeks ago
d0499cda29
KWeather: Split Display module in two sumbodules
blu.256 added 1 commit 4 weeks ago
5cd095385a
Move useIconTheme DCOP call into kcmweatherservice.
Poster
Collaborator

All remaining 'papercuts' seem to be fixed, issue 1 also implemented. Please test and report and also please comment on section naming (previous comment). Thank you!

All remaining 'papercuts' seem to be fixed, issue 1 also implemented. Please test and report and also please comment on section naming (previous comment). Thank you!
Owner

@blu.256 thanks Philippe. I will probably check after the weekend.

@blu.256 thanks Philippe. I will probably check after the weekend.
Owner

BTW, there are some new <property name="accel"> that are probably not desirable.

BTW, there are some new `<property name="accel">` that are probably not desirable.
Poster
Collaborator

BTW, there are some new <property name="accel"> that are probably not desirable.

Must be the result of my using kdevdesigner again. I will remove them. Thank you!

> BTW, there are some new `<property name="accel">` that are probably not desirable. Must be the result of my using `kdevdesigner` again. I will remove them. Thank you!
Owner

Must be the result of my using kdevdesigner again. I will remove them. Thank you!

Good, then I will do a test.

> Must be the result of my using `kdevdesigner` again. I will remove them. Thank you! Good, then I will do a test.
Owner

Maybe naming sections "* Config" in a configuration dialog is a little superfluous? What about:

a. Applet
b. (Weather) Service
c. Stations

Sounds good, since they are already displayed in the config modules. So:

  1. Applet
  2. Service
  3. Stations
> Maybe naming sections "* Config" in a configuration dialog is a little superfluous? What about: > > > a. Applet > > b. (Weather) Service > > c. Stations Sounds good, since they are already displayed in the config modules. So: 1. Applet 2. Service 3. Stations
blu.256 force-pushed feat/kweather-std-icons from 5cd095385a to 6bca9050ac 4 weeks ago
Poster
Collaborator

BTW, there are some new <property name="accel"> that are probably not desirable.

Fixed.

> BTW, there are some new `<property name="accel">` that are probably not desirable. Fixed.
MicheleC reviewed 4 weeks ago
DCOPRef ws( "KWeatherService", "WeatherService" );
DCOPReply reply = ws.call( "useIconTheme", useIconTheme );
if( ! reply.isValid() )
kdDebug(12004) << "[kcmweatherservice::updateIconTheme] DCOP call failed" << endl;
Poster
Owner

line 146: indentation is off. Also please add braces { }

line 146: indentation is off. Also please add braces { }
blu.256 marked this conversation as resolved
MicheleC reviewed 4 weeks ago
settingsDialog->addModule( "kcmweather.desktop" );
settingsDialog->addModule( "kcmweatherservice.desktop" );
settingsDialog->addModule( "kcmweatherapplet.desktop" );
Poster
Owner

line 164-166: please check indentation.

line 164-166: please check indentation.
blu.256 marked this conversation as resolved
MicheleC reviewed 4 weeks ago
dockWidget->setViewMode(mViewMode);
if ( !mWeatherService )
initDCOP();
Poster
Owner

line 338: braces would be good

line 338: braces would be good
blu.256 marked this conversation as resolved
MicheleC reviewed 4 weeks ago
return kapp->iconLoader()->loadIcon(
iconFileName(stationID),
TDEIcon::Desktop
);
Poster
Owner

line 153: please check indentation.

line 153: please check indentation.
blu.256 marked this conversation as resolved
MicheleC requested changes 4 weeks ago
MicheleC left a comment

Some papercut changes required. Other than that is seems good, now will proceed with a functionality test.

Owner

Result of testing.

Without restarting the weather service, with masalla icons and "use system theme" selected:

  1. applet, right click -> Show report: the icon is missing
  2. applet, right click -> Configure: only the service module is shown
  3. konqueror weather bar -> configure: service and stations module are shown, but service icon shows a gear, which does not look very good (it gives the idea an icon is missing, IMO).
  4. konqueror weather bar -> configure: after changing from system icons to KWeather theme, icons are not updated. Clicking update does not help either.

After a reboot, same test gives:

  1. icon not shown with masalla icon set. Icon ok if "use classic KWeather theme" is selected.
  2. service module is now shown correctly. I would order the three modules as in "applet, service, stations" rather than "applet, stations, service".
    "applet" icon shows the TDE "T menu" icon. "service" icon is a gear
  3. same as above. Again I would place "service" before "stations", IMO
  4. same as above, changing icon them from Konqueror sidebar settings makes no difference.
    4.1) Changing the icon theme from the applet settings, updates the icon of the display station in the applet and the icons shown in the Konqueror side bar.
    4.2) The same action of 4.1 does not update the icons in the "stations" service module. It is necessary to click the "update all" button to see the new icons. It would be good to update them automatically as it happens on the Konqueror sidebar instead.
Result of testing. Without restarting the weather service, with masalla icons and "use system theme" selected: 1) applet, right click -> Show report: the icon is missing 2) applet, right click -> Configure: only the service module is shown 3) konqueror weather bar -> configure: service and stations module are shown, but service icon shows a gear, which does not look very good (it gives the idea an icon is missing, IMO). 4) konqueror weather bar -> configure: after changing from system icons to KWeather theme, icons are not updated. Clicking update does not help either. After a reboot, same test gives: 1) icon not shown with masalla icon set. Icon ok if "use classic KWeather theme" is selected. 2) service module is now shown correctly. I would order the three modules as in "applet, service, stations" rather than "applet, stations, service". "applet" icon shows the TDE "T menu" icon. "service" icon is a gear 3) same as above. Again I would place "service" before "stations", IMO 4) same as above, changing icon them from Konqueror sidebar settings makes no difference. 4.1) Changing the icon theme from the applet settings, updates the icon of the display station in the applet and the icons shown in the Konqueror side bar. 4.2) The same action of 4.1 does not update the icons in the "stations" service module. It is necessary to click the "update all" button to see the new icons. It would be good to update them automatically as it happens on the Konqueror sidebar instead.
Owner

In summary:
a. the code changes means this PR will only go into R14.1.0 and not R14.0.x
b. functionality has improved and we are almost there, some parts still need some touch up
c. on icons and module orders, I only provided my opinion, would be good to hear also others (@SlavekB)
d. a question remains on how to inform the user that a restart of the weather service is required to avoid broken experience after an update, in case the update is done in a live session.

In summary: a. the code changes means this PR will only go into R14.1.0 and not R14.0.x b. functionality has improved and we are almost there, some parts still need some touch up c. on icons and module orders, I only provided my opinion, would be good to hear also others (@SlavekB) d. a question remains on how to inform the user that a restart of the weather service is required to avoid broken experience after an update, in case the update is done in a live session.
blu.256 added 1 commit 3 weeks ago
471cad022a
Code style improvements.
Poster
Collaborator

@MicheleC Thank you once again for taking the time to do the testing.

applet, right click -> Show report: the icon is missing
icon not shown with masalla icon set. Icon ok if "use classic KWeather theme" is selected.

Masalla has vector SVG icons, right? AFAICT it is an issue with TDEHTML, where using <img src='[SVG icon path]'> does not work with vector icons. You can check this by opening the about:konqueror URL in Konqueror. It should show the welcoming page but with a SVG icon set the icons will be missing.

"applet" icon shows the TDE "T menu" icon. "service" icon is a gear

Yes, the icons are "kicker" and "gear" (I couldn't think of better icon choice).

Again I would place "service" before "stations", IMO

OK, let's try this.

same as above, changing icon them from Konqueror sidebar settings makes no difference.
The same action of 4.1 does not update the icons in the "stations" service module. It is necessary to click the "update all" button to see the new icons. It would be good to update them automatically as it happens on the Konqueror sidebar instead.

Good points, will look into these.

a question remains on how to inform the user that a restart of the weather service is required to avoid broken experience after an update, in case the update is done in a live session.

Couldn't we automate this somehow? Like a post-update hook? I'm no packager so I don't know whether this is possible with all package managers.

@MicheleC Thank you once again for taking the time to do the testing. > applet, right click -> Show report: the icon is missing > icon not shown with masalla icon set. Icon ok if "use classic KWeather theme" is selected. Masalla has vector SVG icons, right? AFAICT it is an issue with TDEHTML, where using `<img src='[SVG icon path]'>` does not work with vector icons. You can check this by opening the `about:konqueror` URL in Konqueror. It should show the welcoming page but with a SVG icon set the icons will be missing. > "applet" icon shows the TDE "T menu" icon. "service" icon is a gear Yes, the icons are "kicker" and "gear" (I couldn't think of better icon choice). > Again I would place "service" before "stations", IMO OK, let's try this. > same as above, changing icon them from Konqueror sidebar settings makes no difference. > The same action of 4.1 does not update the icons in the "stations" service module. It is necessary to click the "update all" button to see the new icons. It would be good to update them automatically as it happens on the Konqueror sidebar instead. Good points, will look into these. > a question remains on how to inform the user that a restart of the weather service is required to avoid broken experience after an update, in case the update is done in a live session. Couldn't we automate this somehow? Like a post-update hook? I'm no packager so I don't know whether this is possible with all package managers.
Owner

Masalla has vector SVG icons, right? AFAICT it is an issue with TDEHTML, where using does not work with vector icons. You can check this by opening the about:konqueror URL in Konqueror. It should show the welcoming page but with a SVG icon set the icons will be missing.

Wow, I never noticed that before. Yes, I can confirm that icons are missing in about:konqueror.

Yes, the icons are "kicker" and "gear" (I couldn't think of better icon choice).

For applet: I think since this is a weather applet, some icons related to weather would be more appropriate? Maybe some of the weather status icons?

For service: I find the "gear" a bit empty compated to the others, but fair enough. I will leave the choice to you.

Couldn't we automate this somehow? Like a post-update hook? I'm no packager so I don't know whether this is possible with all package managers.

Yup, probably that is what we will need to do on the debian packaging files. For other distros, the packagers will have to do something like that too.

> Masalla has vector SVG icons, right? AFAICT it is an issue with TDEHTML, where using <img src='[SVG icon path]'> does not work with vector icons. You can check this by opening the about:konqueror URL in Konqueror. It should show the welcoming page but with a SVG icon set the icons will be missing. Wow, I never noticed that before. Yes, I can confirm that icons are missing in about:konqueror. > Yes, the icons are "kicker" and "gear" (I couldn't think of better icon choice). For applet: I think since this is a weather applet, some icons related to weather would be more appropriate? Maybe some of the weather status icons? For service: I find the "gear" a bit empty compated to the others, but fair enough. I will leave the choice to you. >Couldn't we automate this somehow? Like a post-update hook? I'm no packager so I don't know whether this is possible with all package managers. Yup, probably that is what we will need to do on the debian packaging files. For other distros, the packagers will have to do something like that too.
Owner

Good points, will look into these.

@blu.256
Just checking where we are with this and that you are not waiting for feedback from my side 😄

> Good points, will look into these. @blu.256 Just checking where we are with this and that you are not waiting for feedback from my side :smile:
Poster
Collaborator

So, status update: I'm not working on this or the other relevant PR in KMix right now (other priorities, less time). I also don't currently need any feedback, but thank you for asking anyway :-).

So, status update: I'm not working on this or the other relevant PR in KMix right now (other priorities, less time). I also don't currently need any feedback, but thank you for asking anyway :-).
Owner

So, status update: I'm not working on this or the other relevant PR in KMix right now (other priorities, less time). I also don't currently need any feedback, but thank you for asking anyway :-).

Ok, no problem and no rush. Was just making sure we were not waiting on each other 😄

> So, status update: I'm not working on this or the other relevant PR in KMix right now (other priorities, less time). I also don't currently need any feedback, but thank you for asking anyway :-). Ok, no problem and no rush. Was just making sure we were not waiting on each other :smile:
blu.256 force-pushed feat/kweather-std-icons from 471cad022a to 4440576046 6 days ago
blu.256 changed title from KWeather XDG icon names and icon theme support to WIP: KWeather XDG icon names and icon theme support 6 days ago
blu.256 force-pushed feat/kweather-std-icons from 4440576046 to aaa7f2013f 6 days ago
Poster
Collaborator

Ready for testing! Test for:

  • Whether the correct icon theme is used on startup;
  • Whether the icon theme can be changed via the Sidebar;
  • Whether the section order and icons in the settings dialog look okay.
Ready for testing! Test for: * Whether the correct icon theme is used on startup; * Whether the icon theme can be changed via the Sidebar; * Whether the section order and icons in the settings dialog look okay.
blu.256 requested review from MicheleC 6 days ago
Owner

@blu.256
The latest update looks very good. There is only one point still open, which I mentioned here as point 4.2.
If we change the icon themes, the icons in the konqueror sidebar gets updated automatically, but the ones in the stations module in the configuration page don't. It is necessary to click the "update all" button to see the new icons in the stations page too.

Once this gets fix, we are ready for merging :-)

@blu.256 The latest update looks very good. There is only one point still open, which I mentioned [here](https://mirror.git.trinitydesktop.org/gitea/TDE/tdetoys/pulls/18#issuecomment-17126) as point 4.2. If we change the icon themes, the icons in the konqueror sidebar gets updated automatically, but the ones in the stations module in the configuration page don't. It is necessary to click the "update all" button to see the new icons in the stations page too. Once this gets fix, we are ready for merging :-)

Reviewers

SlavekB requested changes 1 month ago
MicheleC was requested for review 6 days ago
This pull request is marked as a work in progress.
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.