Fix tooltip and show weather condition correctly #103
Conversation
| @@ -156,5 +156,5 @@ extern "C" BView* instantiate_deskbar_item(float maxWidth, float maxHeight); | |||
| BView* | |||
| instantiate_deskbar_item(float maxWidth, float maxHeight) | |||
| { | |||
| return new ForecastDeskbarView(BRect(0, 0, maxWidth - 1, maxHeight - 1)); | |||
There was a problem hiding this comment.
this should use something like std::min(maxWidth, maxHeight), to compute the largest square that can fit in the space allowed by DeskBar.
This makes the replicant usable no matter what layout DeskBar decides to use in future versions.
There was a problem hiding this comment.
Thanks @pulkomandy, I've updated the code
dospuntos
force-pushed
the
deskbar_replicant_fixes_v2
branch
from
71f6d3c to
dfd4f69
Compare
There was a problem hiding this comment.
The replicant appears to be stuck on "Condition: Loading..."
It's also too small, still getting the icon in size kSizeDeskBarIcon, see https://github.com/HaikuArchives/Weather/blob/master/Source/ForecastView.cpp#L695
The menu item should be sentence case: "Deskbar replicant".
Include a re-generated en.catkeys.
Source/ForecastDeskbarView.h
Outdated
| @@ -18,7 +18,7 @@ | |||
| class ForecastDeskbarView : public BView | |||
| { | |||
|
|
|||
| public: | |||
| public: | |||
There was a problem hiding this comment.
Remove stray tab (in the line above too)
There was a problem hiding this comment.
I updated the PR based on your comments. The main branch was updated recently, so I ditched my first PR and created a new one.
- Icon size is now correct
- Changed menu item to sentence case
- Regenerated en.catkeys
- Removed stray tab
dospuntos
force-pushed
the
deskbar_replicant_fixes_v2
branch
2 times, most recently
from
77b33d8 to
b84d3b6
Compare
|
The replicant icon size appears to be correct now, resized with tray size. |
|
To clarify, is the wrong weather and "condition loading..." caused by this change, or is it an independant regression? If it's the latter, maybe we can merge this still? |
|
Both. :) |
| SetToolTip(weatherDetailsText.String()); | ||
| weatherDetailsText.SetToFormat(B_TRANSLATE("Temperature: %s\nCondition: %s\nLocation: %s"), | ||
| FormatString(fForecastView->Unit(), fForecastView->Temperature()).String(), | ||
| fForecastView->GetStatus().String(), |
There was a problem hiding this comment.
Here the "condition" (sunny, rainy, ...) was replaced by the status (loading, up to date). I guess this should be reverted, but I don't know if there is a way to convert the condition into a string.
There was a problem hiding this comment.
Sorry for the late reply :(
fForecastView->GetStatus() will return the status (loading, no network, connection error), but also the correct condition (sunny, rainy etc.) as a translatable string when available.
I'm puzzled as to why @humdingerb isn't getting the correct forecast. On my machine it works (both VM and natively), but after changing the code I have to open the app, enable and disable the Desktop replicant, and then restart the app to make sure the replicant is running the new code. I also uninstalled the Weather app from HaikuDepot to avoid any conflicts.
There was a problem hiding this comment.
No idea either. I checked out PR #103, built cleanly and there is no other Weather binary on the system. Still the same: the Deskbar replicant always shows the sun and "Condition" says "Loading...".
dospuntos
force-pushed
the
deskbar_replicant_fixes_v2
branch
from
b84d3b6 to
41c4131
Compare
I think it is an independent regression, my changes are only related to the deskbar icon size and other visuals. WRT the eternal "Loading..." status, I have confirmed that @humdingerb is right, on a clean install the deskbar never shows the actual weather data. |
This PR fixes a few minor issues:
There is a pending issue with scaling, the constant
kSizeDeskBarIconinForecastView.hwill cause the generated icon to 16x16 pixels. It can be scaled, but that makes it look blurred. The icon should be generated to the value ofmaxHeightfrominstantiate_deskbar_item(float maxWidth, float maxHeight)