search

szanni / slideextract

extract slides from videos
https://szanni.org/slideextract/
BSD 2-Clause "Simplified" License
61 stars 14 forks source link

Fixed region of interest handling in GUI #2

Closed dunhill closed 2 years ago

dunhill commented 2 years ago
szanni commented 2 years ago

Thanks dunhill for the PR. Where does the idea come from that something is broken? True, the man page does not specifically mention a point1:point2 but the x1.y1:x2.y2 description suggests that the program expects two points, no?

I would accept a clarification in the help message AND man page (which is missing from the PR). I am not willing to break the API, which this PR is doing. I am however open to the idea of adding a new option to specify a point and width+height. Either via a new flag or (porbably preffered) different syntax like P1X.P1Y:W+H or P1X.P1Y:WxH. I would like something that mirrors whatever syntax ImageMagick uses for cases like these.

dunhill commented 2 years ago

Hi @szanni. Great to see such a prompt feedback. Will try to catch up with your speed of response.

I should have split my PR into few separate once. The main change that motivated me to even open it was just a correction of a help message done in the first commit. As written in the main.c line 103:

if (sscanf(optarg, "%d.%d:%d.%d", &roi.x, &roi.y, &roi.width, &roi.height) != 4)

So that my Commit 1 was only updating the docs in line 73 of that same file to reflect the actual behavior.

dunhill commented 2 years ago

The subsequent commits were aimed on fixing the ROI selection with UI. On my system (and here I should admit that I have no clue if it works differently on any other one) the mouse Events come into such a sequence, that code in the master branch was not recording the point where the mouse was first pressed, as it was already expecting a flag of "the mouse is pressed" to be already raised. But that flag got raised (again on my system) only with the subsequent mouse event. That is the explanation of the commits 2-5.

szanni commented 2 years ago

if (sscanf(optarg, "%d.%d:%d.%d", &roi.x, &roi.y, &roi.width, &roi.height) != 4)

Wow, thanks for sending that in. It's been years, I was completely unaware the docs were describing something different to the actual workings. This is indeed a bug! I was only testing the -r option with coordinates that the gui prints... which are actually x.y:w.h. I will fix this and probably have to break API after all and release version 0.3.0. See #4.

The subsequent commits were aimed on fixing the ROI selection with UI. On my system (and here I should admit that I have no clue if it works differently on any other one) the mouse Events come into such a sequence, that code in the master branch was not recording the point where the mouse was first pressed, as it was already expecting a flag of "the mouse is pressed" to be already raised. But that flag got raised (again on my system) only with the subsequent mouse event. That is the explanation of the commits 2-5.

Would you be willing to report that as an actual bug? This will hopefully make things reproducible by me and others. I have no problems whatsoever on my system.

I guess I should write a CONTRIBUTING.md as well to explain preferred contributing workflow.

Thanks so much so far!