Opened 10 years ago

Closed 9 years ago

#8356 closed defect (wontfix)

Mythweather: need to be less US-centric

Reported by: Alec leamas <gmail: leamas.alec> Owned by: beirdo
Priority: minor Milestone: unknown
Component: Plugin - MythWeather Version: Master Head
Severity: low Keywords: mythweather
Cc: Ticket locked: no

Description

Mythweather is as of today to US-centric to be used successfully for some of us living elsewhere. Problems include:

  • Use of 12-hour clock.
  • Display of pop data, which not is available everywhere.
  • Units (e. g. wind directions) are not translated.
  • The gust wind speed not available everywhere is used.
  • Windspeed is displayed in mph or km/h, some of us uses other units.

Attaching a patch serie, comment in each patch. In short:

  • localtime: use locale-specific clock in 18-hour forecast
  • precip: Use precipitation (mm) if pop is unavailable.
  • new-grabber-if: simplify interface plugin <-> grabber.
  • wind-source: use average wind if gust is unavailable.
  • wind-units: use knots, m/s, beaufort etc for wind speed.
  • script-fixes*. Updates to use new interface, not required.

Attachments (9)

localtime.patch (2.3 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
precip.patch (6.9 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
new-grabber-if.patch (11.1 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
wind-source.patch (2.7 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
wind-units.patch (3.7 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
script-fixes.patch (2.4 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
script-fixes-1.patch (4.5 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
script-fixes-2.patch (4.5 KB) - added by Alec leamas <gmail: leamas.alec> 10 years ago.
patches.tar (50.0 KB) - added by Alec leamas <leamas.alec.%alfa%.gmail.com> 10 years ago.
Revised patches

Download all attachments as: .zip

Change History (29)

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: localtime.patch added

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: precip.patch added

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: new-grabber-if.patch added

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: wind-source.patch added

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: wind-units.patch added

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: script-fixes.patch added

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: script-fixes-1.patch added

Changed 10 years ago by Alec leamas <gmail: leamas.alec>

Attachment: script-fixes-2.patch added

comment:1 Changed 10 years ago by stuartm

Status: newinfoneeded_new

A couple of quick comments after a brief look.

1) We should use the time format specified by the user in the settings instead of the localtime patch 2) For QString, = "" should be replaced with .clear() and == "" should use .empty() instead. Each of these are more efficient.

comment:2 Changed 10 years ago by Alec leamas <gmail: leamas.alec>

OK, I can try to fix that. Do you know under which key the user stores her time settings? Will be away for a week from tomorrow, so this might take a while.

comment:3 Changed 10 years ago by robertm

As I mentioned in the last ticket you opened on the idea, I am not ok with overloading amount of precipitation with percent chance of precipitation. I think precip.patch is a hack that would be ugly to maintain and unpredictable. If you want to parse two values, parse two keys. (IMO)

comment:4 Changed 10 years ago by robertm

I'm also not crazy about requiring a text label for one random piece of information, this destroys flexibility for the themer-- what if one wants to use images for labels, or no labels at all?

comment:5 Changed 10 years ago by Alec leamas <gmail: leamas.alec>

I'm really open to how to resolve these issues. So far, I haven't really understood other alternatives, though.

Precip: I have tried to resolve the issue without modifying the UI. Is that the wrong approach? As long as the data should be delivered in the "pop" data item transparent to the UI, I don't see how to fix this in a more clean way. I can see that this code has drawbacks. But it solves a real problem, and if this isn't the right way to do it I would appreciate a hint how to make it in a better way :-) I already know it's not the most elegant piece of sw... OTOH, I have explored some other approaches which definitely was worse.

text label: I see your point. The only solution I can think of is to either accept that this is a US plugin. Or maybe change the field to display the (average) wind speed. If anything is available, this one is. The gust wind speed is not as common (which is sad, I see the point of using gust speed). If it always displays average wind, there is no need for a text label. But otherwise, the UI should somehow hint what the value is all about IMHO.

Above all I miss a common understanding if this US-centric behaviour is a problem... Is my basic idea to try to make the plugin usable in a wider context wrong?

comment:6 Changed 10 years ago by Alec leamas <leamas.alec.%alfa%.gmail.com>

Sorry, other things takes their time... Attaching new patch series, this time as a tar archive to decrease noise. Some changes:

  • localtime now uses timeFormat setting instead of locale.
  • precip: weatherUtils::DataMap? now has more reasonable semantics with a 'previousKey' instead of an ad-hoc 'formatHint' string.
  • precip: Now uses generic 'precip-*' data items instead of possibly overloading pop with precipitation level, making naming more consistent.
  • wind-source: Dynamic text label removed (small UI change).
  • wind-units: Uses gContext->GetSetting? instead of direct DB calls.
  • All == "" and = "" should be gone.
  • Updated details in patch comments
  • New patch order, see file series.

I have not updated against latest trunk at this point.

Looking forward to more comments...

Changed 10 years ago by Alec leamas <leamas.alec.%alfa%.gmail.com>

Attachment: patches.tar added

Revised patches

comment:7 Changed 10 years ago by robertm

Status: infoneeded_newnew

comment:8 Changed 10 years ago by anonymous

There is a plain bug in wind-units (handling of bad unit). And the comment in localtime :formatTime is wrong. Deferring fix until more comments. --alec

comment:9 Changed 9 years ago by beirdo

Owner: changed from Isaac Richards to beirdo
Status: newassigned

comment:10 in reply to:  9 Changed 9 years ago by beirdo

I will comment on these as I work with them one at a time.

localtime.patch:

  • as provided, this does not compile as the context should be gGlobalContext, not gContext
  • this may be a good idea, but is not implemented in a way that works across the different scraper results. It only works with the us_nws scraper(s) as best I can tell. The others all present different input time formats, usually including dates.
  • Assuming that the user will not want the date as part of the display, especially when the times are over more than one day, is shortsighted. This is bound to cause confusion.

Patch rejected.

comment:11 Changed 9 years ago by beirdo

script-fixes.patch:

  • I don't see why you'd want to force 2 decimal places on all temperatures. That seems like overkill. Perhaps one, but even that's overkill, really.

script-fixes-1.patch:

  • I'm not sure what you are trying to do here. It makes no sense to me to mash the metric and non-metric together. The system's setup to show one or the other, not both, and I seriously doubt many people would ever want both.

script-fixes-2.patch:

  • much of this is predicated on the changes in script-fixes-1.patch
  • The idea of failing gracefully on missing data is good. I'll consider this, but the implementation leaves a lot to be desired
  • using printf in perl is not often needed, especially to print strings.

Patches rejected. I will look into making the scripts fail out more gracefully however.

comment:12 Changed 9 years ago by beirdo

precip.patch:

  • as indicated above by robertm, this has been rejected once already. We can't share that data item on the UI, some themes may have it labelled in a way that will look stupid if you do.

Patch rejected.

comment:13 Changed 9 years ago by beirdo

new-grabber-if.patch:

  • No thanks. I don't see the allure of replacing easy conversions done in perl with C++ code that may or may not be needed anyways. The system is designed to allow metric or non-metric units... why change it?

Patch rejected.

comment:14 Changed 9 years ago by beirdo

I will get back to this tomorrow. I need a break.

Thank you for your efforts, but so far, what I have seen is too problematic to be put in right now, or is going the wrong direction. The concepts may be good though, and when I create the list of things to work on in mythweather, I will definitely consider adding some of this. In the meantime, my plan is to get mythweather fully operational, and abiding by the TOS of the providers we use.

comment:15 Changed 9 years ago by beirdo

the comment on localtime.patch should read gCoreContext, not gGlobalContext. Typo.

See [24623] and [24624]

comment:16 Changed 9 years ago by beirdo

wind-source.patch:

  • The wind_spdgst is supposed to contain *BOTH* average and gust speeds. If the scrapers are written correctly, then this would not be an issue.
  • I would rather see the combining done in the scrapers or completely in the C++. Right now it's done in the scrapers, and I'm not ready to be changing that yet.
  • I will work with the themers to tweak the themes to indicate "Wind" when using wind_spdgst. Currently nothing actually uses wind_speed or wind_gust
  • There are many more themes than what was changed here, and all need modification

Patch rejected.

I also fixed the last "kph" text to say "km/h"

comment:17 Changed 9 years ago by beirdo

kph->km/h change is in [25146]

comment:18 Changed 9 years ago by beirdo

Keywords: i18n removed

comment:19 Changed 9 years ago by beirdo

And finally... wind_units.patch:

  • converting mph->m/s is not a no-op
  • converting mph->mph IS a no-op (you got em backwards)
  • at this time we are not adding new settings unless absolutely necessary
  • I would like to keep the conversions in one place. Right now that place is the scrapers. I will definitely keep this code around for reference, especially the beaufort scale part. Once I'm ready to implement the new settings, I will be referencing it.

Patch rejected.

comment:20 Changed 9 years ago by beirdo

Resolution: wontfix
Status: assignedclosed

Thanks for the ideas, I will reference your patches later on, for sure. Just not ready to be implementing these things, and not in this way. Sorry.

Note: See TracTickets for help on using tickets.