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

Add touch option to hide cursor on any touch input #50

Closed
wants to merge 1 commit into from

Conversation

frink
Copy link

@frink frink commented Aug 18, 2019

I found this fork with additions to your project from a link in a random Stack Overflow post. It was 6 months old with no PR yet, so I thought it would be good to create one and get the code merged in. Seems like a useful addition...

@Airblader
Copy link
Owner

Thanks, but content from SO is to be assumed to be licensed CC BY-SA 3.0, I cannot simply merge this in, sadly. You'd have to ping the author on SO. Do you have a link to it?

@Airblader Airblader closed this Aug 18, 2019
@frink
Copy link
Author

frink commented Aug 18, 2019

I think you misunderstood...

I found a reference link on StackOverflow that pointed to this fork on GitHub while trolling for answers on a touchscreen problem. I didn't write the code or copy it. I merely noted that a PR should not be done and the code deserved to be pushed back upstream...

To be clear, the PR is from a fork of your own repository hosted on GitHub with the same MIT license and even the same copyright notice. Everything is license MIT - Copyright Ingo Bürk. I've edited my comments above to make it clear that no code is license CC-BY-SA 3.0. From a license perspective, I do not believe there is any problem in merging this code.

SEE: https://github.com/nowrep/unclutter-xfixes/blob/master/LICENSE

I reviewed the code and it seemed reasonable so I created a pull request to help both of you and @nowrep connect and get a more useful project rather than divergent forking hell that can result from smaller projects on GitHub. Was just trying to help you harvest some changes back upstream.

Sorry for the confusion.
Cheers!

@Airblader
Copy link
Owner

Ah, OK, sorry for the misunderstanding then. I will reopen for now and have to look into this later as I'm quite busy right now. Thanks for considering to upstream such changes, though, it's much appreciated!

@Airblader Airblader reopened this Aug 19, 2019
@frink
Copy link
Author

frink commented Aug 21, 2019

It looks pretty simple. I don't see any real conflicts in the source. It's just git getting confused I think...

@Airblader
Copy link
Owner

Given that the remote HEAD is behind, I think git is right here and this needs to be rebased.

@@ -93,6 +94,7 @@ static void parse_args(int argc, char *argv[]) {
{ "jitter", required_argument, 0, 0 },
{ "exclude-root", no_argument, 0, 0 },
{ "ignore-scrolling", no_argument, 0, 0 },
{ "touch", no_argument, 0, 0 },
Copy link
Owner

Choose a reason for hiding this comment

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

"touch" somewhat suggests that it simply includes touch events, but this isn't the case; rather, it does the opposite of what we do for other input events. So I think it'd be better if we could name this --hide-on-touch (and everywhere else accordingly)

@@ -185,7 +190,7 @@ static void parse_args(int argc, char *argv[]) {
}

static void print_usage(char *argv[]) {
fprintf(stderr, "Usage: %s [--timeout <n>] [--jitter <radius>] [--exclude-root] [--ignore-scrolling] [-b|--fork] [-v|--version] [-h|--help]", argv[0]);
fprintf(stderr, "Usage: %s [--timeout <n>] [--jitter <radius>] [--exclude-root] [--ignore-scrolling] [--touch] [-b|--fork] [-v|--version] [-h|--help]", argv[0]);
Copy link
Owner

Choose a reason for hiding this comment

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

Please also update the man page to document this option properly.

@frink
Copy link
Author

frink commented Aug 26, 2019

Should I create a fork to make these changes since I don't have access to @nowrep's source?

@Airblader
Copy link
Owner

Yeah, you'll have to make a fork for that. :-)

@frink
Copy link
Author

frink commented Sep 3, 2019

Ok will do this week.

@chocolatemelt
Copy link

Is this still happening? The touch option is super handy, but it doesn't seem to be merged upstream yet. I don't mind making these fixes if Frink hasn't already.

@Airblader
Copy link
Owner

From my side everything's ready, the PR just has to be updated so it can be merged.

@nowrep
Copy link
Contributor

nowrep commented Sep 19, 2019

Opened new pull request #52

@Airblader Airblader closed this Sep 24, 2019
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.

5 participants