librehat / com.librehat.yahooweather

[DEPRECATED] Yahoo! Weather Widget for Plasma Desktop
https://www.kde-look.org/p/998912/
GNU General Public License v3.0
23 stars 11 forks source link

Tray icon+query retries #66

Closed smitgd closed 8 years ago

smitgd commented 8 years ago

This merges my (previously rejected as hack) branch "handle-query-retires" pr #48 with branch "tray-icon-fix2" pr #62. I'm not sure what about it you thought was a hack but you did mention that I used some magic numbers for "ready" state variables that I have fixed. Otherwise, this seem to work well now and handles error situations such as startup with network down, network going down before a query, network returning, bad woeid at startup or running with bad woeid configured. Both tray icon mode and full mode work correctly and consistently and display the error information (with click on tray icon). The main change is that the 1sec timer for setting the icon/tool-tip did not always actually set the data (maybe m_isbusy/hasdata race?). My proposed fix is a new function to replace the timer that does the same thing as the timer's onTriggered activity and called where "hasdata" is set. This function now also is used to set "hasdata" by passing true or false as a parameter instead of setting it directly. Note: I don't know if you noticed but middle click on tray icon does a refresh.

smitgd commented 8 years ago

I looked closer at why the 1 sec timer did not always work correctly for me. Visibility of the tray icon I made dependent on hasdata, networkError and m_isbusy. When m_isbusy goes false, it causes the icon to be set visible. However, at the time m_isbusy goes false, the timer has yet to timeout and no plasmoid.icon has been set so only a blank space appears (no icon). So even though the icon is eventually set by the timer (within 1s) it does not appear. This could be seen if the widget is started with a bad woeid configured. Although the icon is blank, the correct tool tip displays, I assume, since tool tips have no visibility parameter. The general solution is to make sure the icon/tooltips are set before the variables that affect visibility. This PR almost does that but not quite (although I don't see any functional problems). So I will look at this a bit more and possibly submit another PR.

smitgd commented 8 years ago

I just updated this pull request with 2 more commits. I can rebase/squash comments if the final result seems OK. Anyhow, made sure visibility vars are set after icon/tooltip is set (in function backend.setPlasmoidIconAndTips(). Also, set tooltip to contain "errstring" instead of prompt to click on icon to see error.

librehat commented 8 years ago

Seems good to me. I don't have time testing it though, please ensure it works fine in different conditions. Would you also please close the pull requests that is duplicated by this request?

smitgd commented 8 years ago

Tested on 3 systems (fedora, arch and debian) but noticed that tray temperature+icon was very close or went underneath (or overlapped) the adjacent tray item to left when several digits in temperature, e.g., -100 degrees worst case. In CompactWx.qml setting the "Layout.preferredWidth" some larger than default fixes it on all systems and screen resolutions. So added one more commit to this PR. Also, closed the 2 other PRs that are superceded by this one.

librehat commented 8 years ago

Looks good to me.

Thank you!