highgui: Trackbar Improvements

[Ashod]

I've made 3 (small) improvements/fixes to the Trackbar in Highgui:

1. Added new API function for changing the trackbar's max count after creation. Useful for dynamically changing the range.
2. Using the correct, Windows TBBUTTONINFO, instead of the custom one. This fixes incorrect trackbar width due to incorrect struct "version."
3. Replaced the deprecated CreateToolbarEx, which avoids a warning (on newer VS) and a library dependency.

[StevenPuttemans]

Actually the 2.4 branch gets regular merges into the master branch.
What I should do is

• Close down the master branch PR (highgui: Improved Trackbar #3448)
• Update this PR, fixing the reported errors
• Then let this be approved and merged

Subsequently with the follow version release, master will receive fix automatically.

[ilya-lavrenov]

Actually the 2.4 branch gets regular merges into the master branch.

When did you see the last one? This rule doesn't work now.

[ilya-lavrenov]

@Ashod, please, fix compilation issues http://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/1865/steps/compile%20release/logs/stdio

p.s.

This seems tricky with default parameter value (to avoid breaking the API).

C language doesn't have default functions parameters.

[StevenPuttemans]

O so that rule is gone for good? That would be a bummer for future fixes!

[ilya-lavrenov]

unfortunately, looks like yes for now, because we don't have sufficient people resources to do it.

[StevenPuttemans]

But before a new stable branch can be released, it should be done right? What point there is in fixing stuff if it doesn't stay in ? :)

[StevenPuttemans]

Also if core devs of opencv decide not to merge anymore, then a public notice should be put for each bugfix that you need to apply it on both branches. Can you pass on the message?

[Ashod]

I already have a PR for the master branch (#3448) which passes builds and is ready for merge.

Regarding the build error in this one, removing the default parameter values will break existing users' code. Do we really want this patch that much to introduce a breaking change? I'm happy to fix and proceed if that's what we should do.

[StevenPuttemans]

My opinion, if your fix breaks backwards compatibility on 2.4 branch, then it cannot be merged. But that is just my opinion :)

[Ashod]

@StevenPuttemans Agreed. I didn't know 2.4 was built with strict C rules (the same code builds without issue in the master branch).

Let's wait for a second opinion on this. Meanwhile, can we move ahead with #3448 please?

[StevenPuttemans]

I am voluntary checking PRs on my own initiative. I do not have the power to let PRs being merged. For that you need a core developer with rights to do so. It is normal that it takes some time, many of them are doing this next to their full time job.

[Ashod]

@StevenPuttemans Understood. Just was asking for an update, no rush. I can see it assigned to Ilya, but he only commented on this one. I'm sure he'll get to it eventually. Thanks!

[StevenPuttemans]

Actually Ilya just assigns devs to PRs I think, your assignee is @asmorkalov. He will check this suggestion out from the moment he has some spare time for it :)

[Ashod]

@StevenPuttemans Good to know. Thanks.

[asmorkalov]

@Ashod We cannot merge you request into 2.4, because it breaks binary compatibility (new parameter in public API). You can prepare request with bug fixes only without API change.

[Ashod]

@asmorkalov Thanks. What I suspected myself. That's fine.

I can submit a PR with the non-breaking changes. What about #3448?

[vpisarev]

@Ashod, indeed, this breaks binary compatibility with the previous 2.4.x, so this PR cannot be merged. May be, we should add another function to set maximum (and maybe also minimum) trackbar value, as well as the step between subsequent "ticks"? If you go this route, probably we should also modify PR to the master branch.

[Ashod]

@vpisarev Interesting suggestion. Could be done. Any preference as to the name and interface?

I think a new function is cleaner than default value, but depends how fat we want to make the API. So, either we keep this as default value with same API in 3.x only (and make it unavailable in 2.4) or, as you suggest, add a new function for setting the max count.

Should I wait for confirmation or should I set to work?

[StevenPuttemans]

I suggest to go to work ;)

[Ashod]

Will get to it after work... Thanks.

[Ashod]

Done. Builds clean. Please take a look. (Also updated master.)

[StevenPuttemans]

Seems nice! 👍

[asmorkalov]

👍

[Ashod]

What's next?

[StevenPuttemans]

Someone with merge rights will pass by and put your commit in. Be patient, it has been quite calm lately. Probably people taking some holidays or so.

[Ashod]

Thanks Steven. Just wanted to follow up.

[StevenPuttemans]

👍