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

support xml:lang in release notes #113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maxkadushkin
Copy link

following #35, implemented using xml:lang for multilingual releaseNotesLink. language isn't got from OS (need to do it?), but set up by win_sparkle_set_lang.
the order of releaseNotesLink items doesn't matter. will be taken the first item without xml:lang tag and the last with suitable language

@vslavik
Copy link
Owner

vslavik commented Sep 1, 2016

Thanks for your PR! I can’t merge it in its current state, unfortunately, could you please polish it per the inline comments I’ll add in a moment? I’d also appreciate better commit message with accurate title (#35’s one is more meaningful and understandable than the one you used; there’s nothing “dependent” there for one thing) and a description (the caveats about how it behaves present in this PR should be part of the commit message — see http://chris.beams.io/posts/git-commit/ for the rationale.)

src/appcast.cpp Outdated
@@ -256,7 +276,7 @@ void XMLCALL OnText(void *data, const char *s, int len)
const size_t size = ctxt.items.size();

if ( ctxt.in_relnotes )
ctxt.items[size-1].ReleaseNotesURL.append(s, len);
ctxt.items[size-1].ReleaseNotesURL.assign(s, len);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you did this, but you need to avoid concatenating different links in another way. This is wrong, because Expat makes no guarantees about how often does it call this handler. It may decide to split any text block in multiple pieces, hence append. For example, it would be reasonable to clear ReleaseNotesURL when the tag is opened anew.

@maxkadushkin
Copy link
Author

maxkadushkin commented Sep 7, 2016

yes, of course, I'll examine my changes accordigly your comments

@vslavik
Copy link
Owner

vslavik commented Jan 23, 2019

Merge with upstream

This makes merging the PR somewhat messy :( To reiterate, I'd be happy to merge this once the bugs are fixed.

@maxkadushkin
Copy link
Author

yes, of course. i have no PR without changes according your remarks

@vslavik
Copy link
Owner

vslavik commented Jan 24, 2019

So much for your promise in 2016… or the spirit of contributing back :(

@vslavik vslavik reopened this Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants