k-donn / plasmoid-wunderground

A Plasma 5/6 widget for showing data from Wunderground PWS
https://store.kde.org/p/2135799
GNU General Public License v2.0
35 stars 12 forks source link

feat(fullRepp): full representation rework. #31

Closed rliwoch closed 5 months ago

rliwoch commented 2 years ago

add new icons form Weather Icons project https://erikflowers.github.io/weather-icons/ rework full representation to include icons add English translation fix minor display issues

Hey there, I'm on holidays and in my spare time I've been playing around with some improvements to the full representation of the weather widget. Would you be interested in merging these changes in? I don't want to create yet another fork of some software :) Weather icons are open source but I guess we'd need to do license attribution. Let me know what you think, that's the current shape of it: screen-weather2

I'm also playing around with an idea of using delegates rather than plain Repeater in the forecast view. List/GridView would give a bit more control over the elements as oppose to the Repeater - that's not yet included.

k-donn commented 2 years ago

Hello! Thank you so much for using and helping the widget!

This very elegantly displays the information I want and more info I wanted to add (Sun/moon). The UI was very much cobbled together based on whatever tutorials I could find. Adding more icons was also a direction I was moving towards. I will make sure to review the license of the icons!

Also, I was moving towards having more customizability over what properties were shown on the full rep. The use of delegates steps towards that.

Also thanks for catching a spelling mistake in the template.pot file! More internationalization with en_GB is always welcome! Speaking of, I am currently working on bringing in an es_ES translation of this widget. After that, I will go forward with merging these changes.

Overall, this is definitely something I want to bring in. I have found more time recently to work on this project and look forward to making it better!

I will follow up with any commits to add to this PR.

Thank you!

rliwoch commented 2 years ago

I can sympathise with the sentiment of "cobbling things together" when it comes to QML especially with the KDE libs :) There's no single good tutorial for that, which is freaking surprising - it feels almost like KDE folks don't want people to create quality plasmoids. In the last 10 years I had a few attempts to really sit down and nail it but always ended up being frustrated. Qt Creator (and Designer) would help but it cannot understand org.kde.plasma.plasmoid import as that's some dynamic KDE magic library :/ at least I didn't manage to make it work. But enough of that moaning.

I'll add pl_PL and cz_CZ translations as well, originally I'm Polish, but I spent years living in the UK, hence en_GB was the first one I did :) I'd add German as well, but my German is not that great so I'll consult some of my colleagues here. ^ that would be a separate PR.

Thanks for the great work you did starting this project!

rliwoch commented 2 years ago

On the icons license, seems that OFL license doesn't require specific attribution. image

rliwoch commented 2 years ago

hi Kevin, just wanted to let you know that in the end I made so many changes that I decided to publish my own version: https://www.pling.com/p/1756638 It didn't make sense to send PRs which change entire files and you may not want it at all!

Of course I gave you credit for the original version.

As to this PR - it's up to you if you want to merge it or not. Either way is fine for me.

k-donn commented 2 years ago

Ok! I definitely understand wanting to make your own version given how much changed. Your version does have a different creative vision than what I was going for. As long as you are okay with it I still want to merge the changes you proposed!

Thank you again for your interest! KDE always can use more effort into the weather systems.

rliwoch commented 2 years ago

Absolutely, no issues at all. I'm happy to resolve conflicts here too,seeing that you progressed with some changes

k-donn commented 2 years ago

Hello again!

I was running this branch through the formatter on my machine so the code style would be consistent. I also updated the metadata in the translations.

I now would like to push these changes to the pull request branch so they could then be merged into the repo. But, I cannot push to your remote branch.

I believe you need to enable an "Allow edits from maintainers" setting in the pr for this to work.

I understand this method would be the simplest for me to add commits. However, I am unfamiliar with git/GitHub so I could be very wrong!

Thank you for your help!

rliwoch commented 2 years ago

Hey,

you're allowed to push to this branch (checkbox ticked). I mean, normally that would be pull requester responsibility to fix any issues that the maintainer might have with the code, but that's really a rule for large projects. Nevertheless, you should be able to commit to this branch. Can't see anything obvious that might be stopping you.

Are you sure your resolved all conflicts and you're not trying to force push? (git push --force). Force push won't work. What's the error you're getting and the command you're running?

Also I renamed my repository so please double check your remote in git

git remote get-url <name_of_remote_to_my_repo> 

should show:

git@github.com:rliwoch/plasmoid-wunderground-extended.git

please note the repo name is plasmoid-wunderground-extended not just plasmoid-wunderground

If that's the case - you'll need to change the URL to the remote: git remote set-url <name_of_remote_to_my_repo> https://github.com/rliwoch/plasmoid-wunderground-extended.git

By the way - my updated version that I published (not this PR) is completely based on delagates, making it dynamic based on what's in the model array. But that requires a bit more code changes. Just saying in case you're curious how this could be done, you can take a look at my main branch. The idea for the future is that user will be able to select which weather components he's interested in and could select those elements from the config menu.

k-donn commented 2 years ago

Okay! I understand the workflow of PR's better now.

The delegate system is what I would like to move towards. A goal is for this to be more like KDE 4's Customizable Weather Plasmoid.

Thank you so much!

k-donn commented 2 years ago

I highlighted the difference in bracket style which I was looking to fix by committing to the PR's branch. I am still getting an error to commit to it however.

To github.com:rliwoch/plasmoid-wunderground-extended.git
 ! [remote rejected] HEAD -> rliwoch-feature/fullRepresentationMod (permission denied)
error: failed to push some refs to 'github.com:rliwoch/plasmoid-wunderground-extended.git'

This was the error for curiosity's sake.

But, this is a very simple change. If you are willing to hop in and get the brackets consistent, that would be amazing! You have put up with a tremendous amount of me learning new things and that is amazing!

After that, I would then merge in this change.

rliwoch commented 2 years ago

Hey, no worries, will do but over the weekend. I'm snowed under with work atm.