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

New functions for dialog position and german translation #34

Closed
wants to merge 3 commits into from
Closed

New functions for dialog position and german translation #34

wants to merge 3 commits into from

Conversation

jimmytaker
Copy link

We will be incorporating WinSparkle in our product next year. However, we had two requirements which it did not fulfill:

  1. We wanted to be able to display the window centered on the screen. I added the function set_dialog_position(x, y) to offer this.
  2. Our software is German. We wanted WinSparkle in German. To achieve this I extended it by adding support for wxLocale and the function set_locale(const char* lang). I also provide my translation of the GUI in German as a .po file (in correspondence to the wxWidgets translation guidelines).

@vslavik
Copy link
Owner

vslavik commented Oct 28, 2014

Thanks for the PR and sorry for the delay!

The German translation is definitely nice to have, but the approach taken to the other features seems to be needlessly complicated, for the user of WinSparkle to use, to me:

  1. Why having the caller provide a position at all? Why not instead place the window atop the app's main window automatically? You can enumerate process' windows and pick the top (or largest) one automatically and it seems to me that would be the appropriate thing to do pretty much always…
  2. Likewise, why not pick the process' locale and pick the language from it automatically? In this case I can see why there should be away to override this detection with _set_locale, but the default — and right — thing to do should be to pick the process' setting.

I have some nitpicks, too: Would be nice to have proper git commit messages that explain things ("better string" is especially bad), with one commit with one thing (e.g. de translation should be separated) and as a matter of fact, one PR per one thing. There's no reason to include compiled MO files in git. The patch modifies only one of the project files set, without touching the bkl source the file was generated from.

@jimmytaker
Copy link
Author

I did both this way with only one Thing in mind - not Change the current behavior in case the user does not call the functions (do not call the new functions and you have it behaving exactly the way it was). Doing it the way you suggest would Change the current behavior. Since this is not my Project, I did not want to impose changes that I require on everybody.

Apart from that I agree and I think that having it my way on Default (or better said having it exactly how you describe it) completely satisfies our requirements and is pretty much the way anybody would want it.

If you confirm that it is ok to Change Default behavior, I will rework my changes.

Sorry if my commits are not as expressive as you would like them to be. I guess our whole Team is a bit sloppy on the issue and it has caught up with me too.

@vslavik
Copy link
Owner

vslavik commented Oct 29, 2014

Doing it the way you suggest would Change the current behavior. Since this is not my Project, I did not want to impose changes that I require on everybody.

You're too respectful of the current state ;) It isn't perfect (far from it) and if the default behavior is bad (which WinSparkle's positioning of windows current is, no question about that), fixing it is perfectly OK and welcome.

@vslavik
Copy link
Owner

vslavik commented Nov 2, 2014

Apparently replaced by #37 instead of updating this PR → closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants