Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[addons] move changelog to addon.xml #9351

Merged
merged 1 commit into from Jun 12, 2016

Conversation

@tamland
Copy link
Member

tamland commented Mar 14, 2016

This adds a <news> element to addon.xml that replaces changelog.txt to make it possible to inline in gui, same as the current summary/description/disclaimer.

While technically it's exactly the same as the old changelog, hopefully the rename to 'news' will encourage people to write shorter and less technical deceptions.

Note to addon devs: There is no backwards compatibility here, but older versions of kodi are forward compatible so you can start using it side-by-side in old addons right away.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Mar 14, 2016

I'm not sure @MartijnKaijser is gonna like this because right now we are concatenating all addon.xml files together into one big file on the backend and the more "unnecessary" information is stored in addon.xml the bigger that file will be. Ideally we would only extract the relevant information from addon.xml and write it into the repo file but AFAIK that's not the case yet.

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Mar 14, 2016

@Montellese I think he will as it eliminates all the requests for changelog.txt files:) The number of requests is the main issue. This size of addons.xml is not, even less so now that it's gzipped. It's currently less than 500kb and adding the changelogs here wont add much to the compressed size.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Mar 14, 2016

-10 on this
changelog rarely gets downloaded as it's only done when user actually requests it.

the saving of the couple of times the changelog is download compared to the extra stuffing added to addons.xml which will grow even more is just not done.

edit:
actually not sure i really care atm

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Mar 15, 2016

I guess I was wrong. @MartijnKaijser You know that I'm fully aware of those issues. Please don't just say -10 giving no reason without even being interested in discussing anything. I wouldn't try this if I though it was a problem, or without checking beforehand. It isn't. In the scheme of things, as long as people write short change info for for one version, the combined added size of addons.xml from moving changelogs there is completely negligible.

@zag2me

This comment has been minimized.

Copy link
Contributor

zag2me commented Mar 15, 2016

I like the idea of having everything in one place. Keeping things simple should be our goal, but of course the performance issues must be considered. It sounds like @tamland has already thought about it.

@Razzeee will this help with the scraping for the website?

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Mar 15, 2016

I like the idea (actually thaught about exactly this some days ago too :-) )

@BigNoid

This comment has been minimized.

Copy link
Member

BigNoid commented Mar 15, 2016

+1 From me too. Currently we have to do workarounds to make the changelog feel like its part of addon information dialog, with this PR that wouldn't be needed any more.

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Mar 15, 2016

@zag2me doesn't really change anything, as long as still holds the complete changelog. It will probably kill off some requests.

Not sure about the <news> term, but I understand what your trying to do.

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Mar 15, 2016

Reading the code made me realize, that this makes the changelog translateable. So @alanwww1 might want to know this is comming, as he might have to add that to his processing.

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Mar 15, 2016

@Razzeee Well, the idea here is that you should not dump some complete changelog into "news". Nobody cares about some fix you did 5 version ago.. For that we have changelogs and git logs.

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Mar 15, 2016

Alternatively, if we want to really force ^that, we can change the title to include the version like "What's new in version x.y.z" like app store does.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Mar 15, 2016

I like the idea of a 'What's new...' field to display _only _the latest changes in the addon.xml as some changelogs are huge.

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Mar 15, 2016

for reference: we had 410 requests for changelog.txt in total yesterday, which is practically nothing.

edit: that said, sightly increased size of addon.xml doesn't hurt our mirror redirector in any way.

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Mar 16, 2016

Some thoughts.
This read to me like changelogs are gone from inside kodi?
And if you ask me this kind of translation might be a case we don't have yet. Where we have a field that frequently changes, and translators might have problems catching up. Or authors having old news in other languages they don't understand.

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Mar 16, 2016

@Razzeee It's not gone, there's a listitem property for skins to access it so you don't have to go though the hoops opening a dialog and downloading it. That's the point!, in addition to getting rid of the unnecessary complexity both here and on server side.

As for translations I thought the same, which is why I didn't mention it. Basically addon authors will have to wait for translations before updating so it probably not very useful.

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Mar 16, 2016

The thing I'm more concerned about is having a new value each 4 weeks for an addon. Might add a lot of values / changes to Transifex that in the end don't really matter.

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Mar 16, 2016

Then we wont make it translatable for now. Never intended to add that anyway.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Mar 16, 2016

Translating loads of 'Added...', 'Deleted...', 'Fixed...', 'Changed...' is going to be pretty annoying if you ask me.

@tamland tamland force-pushed the tamland:addon_changelog branch 2 times, most recently from b089665 to 96b0134 Jun 4, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Jun 4, 2016

proceeding then...
@phil65 The current text box is a bit low on space for me to throw in the 'news section', so any on where to put it? (im thinking maybe have a button that 'flips page' if something like that is possible?)

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Jun 5, 2016

Hmm I would say just add the infolabel with this PR, I will find a place for it afterwards then.

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Jun 9, 2016

I just took a closer look and my proposal would be to just add those lines in italics either before or behind the addon description (I guess it is intended to only be very few lines long, right?). If needed i could expand that area by one or two lines.
Is that fine with you, @tamland ?

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Jun 11, 2016

@phil65 Some addons already fill the available space for description. Changelogs can definitely get long and in that case it should in some way flip page or be expandable. Example: (written by you;))

v5.0.0
- Krypton-only
- code cleanup
- fix character encoding issues for actor names
- tmdb multisearch fix
- show correct duration values
- fix for wrong genre API data
- WindowManager fix
- prevent JSON calls with invalid dbids
- improved listitem creation
- better label naming for episodes
- set plugincategory infolabel
- some new listings for Trakt

That, plus heading and spacing is 15 lines..

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Jun 11, 2016

Well, I thaught the intention was that the < news > tag contains a shorter version of the changelog (3-4 lines perhaps) so it wouldnt be that much of a problem then? If you think we need more space I will come up with a different solution though.

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Jun 12, 2016

I don't think we should assume all will be that short at least. It's intended to be a replacement so if you have 20 noteworthy feature/fixes it makes perfect sense to list them imo. If so, it should be viewable in its completeness in some way or another... I'll leave you to it;) If necessary we can always go back to the current text dialog

@tamland tamland force-pushed the tamland:addon_changelog branch from 96b0134 to 6284b5e Jun 12, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Jun 12, 2016

jenkins build this please

@tamland tamland merged commit 5413929 into xbmc:master Jun 12, 2016
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
default Build finished.
Details
@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Jun 19, 2016

Just one heads-up, not sure if you already took care of that: At the moment that Transifex bot automatically updates the changelogs from our core add-ons from time to time. Does that need to get de-activated?

@tamland

This comment has been minimized.

Copy link
Member Author

tamland commented Jun 19, 2016

Hm, those changelog writes and version bumps seems pretty pointless as no one will ever get those as updates. I think we should disable that.. Who owns this bot?

@tamland tamland deleted the tamland:addon_changelog branch Jun 19, 2016
@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Jun 19, 2016

@alanwww1 I think?

@jingai

This comment has been minimized.

Copy link
Contributor

jingai commented Sep 3, 2016

I'm a bit late to comment here, but honestly this is awful. Am I the only one that wants to see a complete history of changes for an addon within Kodi?

edit: Thinking out loud.. why not do both, since it seems the bandwidth usage for the changelogs is minimal anyway?

@NedScott

This comment has been minimized.

Copy link
Contributor

NedScott commented Sep 3, 2016

@jingai While this might be a solution for everyone, but I think all add-ons hosted on the Kodi.tv repo basically have a record of all changes on github (be it the author's copy on github, or the team kodi github).

When the source link is filled out in addons.xml that automatically creates a link for addons.kodi.tv and the add-on wiki pages.

@jingai

This comment has been minimized.

Copy link
Contributor

jingai commented Sep 3, 2016

Why not keep both though, especially if not many people even click the Changelog link?

@Vashiel

This comment has been minimized.

Copy link

Vashiel commented Mar 2, 2017

Since people are starting to use Kodi 17, more people (like me) are realising the missing "Changelog" Button. Since i'am little bit to late to say, that it's a awful idea to remove the changelog completely, i will say hopefully it will come back in later releases of Kodi. "Whats new" is a nice idea, but not as a replacement for Changelog. Why not use it as a short version of changelog and keep the changelog button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.