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

cleanup resolutions, drop resoltion info < RES_WINDOW #14264

Merged
merged 1 commit into from Aug 8, 2018

Conversation

Projects
None yet
3 participants
@FernetMenta
Copy link
Member

commented Aug 4, 2018

see title

those resolutions were not used anyway

@Rechi

This comment was marked as resolved.

Copy link
Member

commented Aug 4, 2018

The following variable is now unused and therefore produces an additional compiler warning on most platforms.

RESOLUTION res = CServiceBroker::GetWinSystem()->GetGfxContext().GetVideoResolution();

@FernetMenta FernetMenta force-pushed the FernetMenta:reso branch from 39991e4 to 824aa1e Aug 4, 2018

<addon id="xbmc.gui" version="5.13.0" provider-name="Team Kodi">
<backwards-compatibility abi="5.13.0"/>
<addon id="xbmc.gui" version="5.13.1" provider-name="Team Kodi">
<backwards-compatibility abi="5.13.1"/>

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Aug 4, 2018

Member

bumping the backwards compatibility in beta stage is not something that is acceptable. This will break every skin in repo

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 4, 2018

Author Member

ok, no need to bump backwards compatibility. this change just removes dead code

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Aug 4, 2018

Member

if they keep working as the flag was unused/not working then keeping it at 5.13.0 should be fine.

@FernetMenta FernetMenta force-pushed the FernetMenta:reso branch from 824aa1e to e9cdb63 Aug 4, 2018

@@ -44,7 +44,7 @@
#define ADDON_GLOBAL_VERSION_GENERAL_XML_ID "kodi.binary.global.general"
#define ADDON_GLOBAL_VERSION_GENERAL_DEPENDS "General.h"

#define ADDON_GLOBAL_VERSION_GUI "5.12.0"
#define ADDON_GLOBAL_VERSION_GUI "5.12.1"
#define ADDON_GLOBAL_VERSION_GUI_MIN "5.10.0"

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 4, 2018

Member

The min version has to be bumped for sure, as you removed a function from the public API.
I'm not sure if therefore a patch version bump is enough

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 5, 2018

Author Member

I bumped min version. not critical because this is only used by vnsi.

@FernetMenta FernetMenta force-pushed the FernetMenta:reso branch from e9cdb63 to 8766d88 Aug 5, 2018

@@ -23,52 +23,6 @@ Start doxygen documentation for add-ons always with `///` and on Kodi itself wit
<b>Here an example on add-on about function coding style:</b>

\verbatim
#ifdef DOXYGEN_SHOULD_USE_THIS

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 5, 2018

Member

either add a different example here or remove the lines above too, which mention that an example will follow

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 5, 2018

Author Member

done

#define ADDON_GLOBAL_VERSION_GUI "5.12.0"
#define ADDON_GLOBAL_VERSION_GUI_MIN "5.10.0"
#define ADDON_GLOBAL_VERSION_GUI "5.13.0"
#define ADDON_GLOBAL_VERSION_GUI_MIN "5.13.0"

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 5, 2018

Member

@MartijnKaijser is this allowed in beta phase?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 5, 2018

Author Member

It is only my addon using this and I am ok with this bump.

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Aug 6, 2018

Member

indeed VNSI is the only binary addon that uses the skin files so no objection there

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 6, 2018

Member

If it's okay for you then all fine.
IMO it doesn't matter which addons use it, but the question is if we want to break API or not.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 6, 2018

Author Member

I can leave the interface functions (as dummies what they already are) in and keep the API versions. No strong feelings about this. Most likely they will stay for another 10 years or longer if we don't care now.
Let me know what solution you prefer.

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 6, 2018

Member

I don't block removing them, but only my addon uses it is the wrong explanation for why an API change is allowed or not.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 6, 2018

Author Member

It depends whether we want to establish rules for the sake of having rules or take a pragmatic approach. For what reason would you block an API change if there are no real downsides?

@FernetMenta FernetMenta force-pushed the FernetMenta:reso branch from 8766d88 to a7832e5 Aug 5, 2018

@Rechi

Rechi approved these changes Aug 6, 2018

@FernetMenta FernetMenta force-pushed the FernetMenta:reso branch from a7832e5 to 7576b56 Aug 7, 2018

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

I removed all API changes

@FernetMenta FernetMenta merged commit 00fb04e into xbmc:master Aug 8, 2018

1 check passed

default You're awesome. Have a cookie
Details

@Rechi Rechi added this to the Leia 18.0-beta1 milestone Aug 8, 2018

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