search

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

Re #74: Security error at zorbuth #75

Open smitgd opened 8 years ago

smitgd commented 8 years ago

This adds support for a location string that overrides the WOEID entry. So now can enter a free form location such as "Pine Crest TN" or "London, england". Only when location is left blank is the entered WOEID used in the yahoo query. Otherwise, the location string (that can also be a zipcode) is used in the query. Also, added note that a web search can be done to find alternate sites to obtain WOEID if needed, plus some clean up on configuration labels (periods at end of sentences, etc.). modified: plasmoid/contents/config/main.xml modified: plasmoid/contents/ui/ConfigGeneral.qml modified: plasmoid/contents/ui/Yahoo.qml

librehat commented 8 years ago

I'd prefer if we can have a radio button to choose between WOEID and location. In this way, we only need one text entry in configuration tab. If user changes from WOEID to location (or vice versa), then the entry should clear its text.

smitgd commented 8 years ago

Yes, that does seem better. Will make the change.

smitgd commented 8 years ago

Added the radio buttons to select location vs. woeid and reduced from 2 to 1 the number of text entry fields and it now does double duty as woeid or location depending on radio button selection. Also implemented the blanking feature you requested. However, I also added a feature to restore the blanked content if the original radio button is re-selected. This avoids having the previously saved entry disappear when the other radio button is selected, possibly by accident. Since the previous commit "worked", I did not rebase/squash it out of the history. So this pull req. now contains 2 commits.

smitgd commented 8 years ago

Added one more small feature to this PReq. This causes the label for the woeid/location text field to toggle between WOEID and Location depending on the current radio button selection. Before the label was just the static string "Location or WOEID". Seems to work OK. After adding this, noticed that the link to the zourbuth site wasn't working. I had accidentially removed the onLinkActivated action. So now put it back and link now works again.

librehat commented 8 years ago

Sorry it has been very busy for me recently, I'll try to spare some time to review your code asap

smitgd commented 8 years ago

No problem.

smitgd commented 8 years ago

Added another commit to this PReq which I hope covers your concerns. -gene

librehat commented 8 years ago

I personally feel the logic there is a bit redundant. If we just show an entry, then we don't need to process onClicked event. Instead, we just need to read which radio button is selected, which means no strange bindings and value assignments.

smitgd commented 8 years ago

Have you tried running it with my latest commit and, if so, is the functionality OK with you? Please notice that when the original unselected button is selected the text field clears. But if the original button is re-selected, before Apply or OK clicked, (even if new text is entered) the original text content is restored (the last saved woeid or location). Before I attempt any more optimizations or changes I want to make sure that this functionality is OK. Thanks, -gene