Skip to content

Change confusing logic #1731

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

Merged
merged 3 commits into from
May 13, 2024
Merged

Conversation

Thinner77
Copy link
Contributor

Summary

This PR will simply invert logic of variable _noSignalDetected in V4L2Grabber.cpp and MFGrabber.cpp, as current logic is IMHO very confusing.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of web configuration, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing setups:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's body (e.g. Fixes: #xxx[,#xxx], where "xxx" is the issue number)

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated (docs/docs/en)
  • Related tests have been updated

PLEASE DON'T FORGET TO ADD YOUR CHANGES TO CHANGELOG.MD

  • Yes, CHANGELOG.md is also updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

This is my first PR ever, i'm still trying to understand git, so please be patient.

@Lord-Grey
Copy link
Collaborator

Lord-Grey commented May 3, 2024

@Thinner77 Thanks for your contribution!
Just looking at the current code, I agree that it is confusing.
Nevertheless, I am wondering why not renaming the variable to "isSignalDetected".
That would even be more readable than having a noSignal and false combination. Two times negation to express a positive is hard to read from my perspective...
What do you think?

@Thinner77
Copy link
Contributor Author

Thinner77 commented May 5, 2024

@Lord-Grey Great idea, let's do it. wbr

PS: I've changed indent in MFHGrabber.h, it looks good now in my editor but not here at GH, don't know how to handle this. Commit message was "Change confusing logic #2", now i know it was a bad idea. As i said, i'm still learning how to use git. Sorry.

@Lord-Grey
Copy link
Collaborator

Lord-Grey commented May 9, 2024

@Thinner77 could you have a try updating the indents, please?
I guess in your IDE you have configured tabs displayed as 8 spaces rather than 4 spaces why that happened.

... and do not be to harsh with yourself on git etc.
You will work it out. I had challenges myself too at the beginning.
Happy to receive further contributions 😄

@Thinner77
Copy link
Contributor Author

@Lord-Grey Hi, i reverted the changes to indent, in my editor (PSPad) i see tabs, no spaces. wbr

grafik

@Lord-Grey
Copy link
Collaborator

Lord-Grey commented May 13, 2024

in my editor (PSPad) i see tabs

My comment was not that tab are replaced by spaces, but how tabs are visualised as spaces.

You should have tabs represented as 4 spaces, as in:

image

Anyway... thanks for correcting the indents and for your contribution.

@Lord-Grey Lord-Grey merged commit f6cc926 into hyperion-project:master May 13, 2024
@Thinner77 Thinner77 deleted the revlogic branch May 13, 2024 21:06
@Thinner77
Copy link
Contributor Author

@Lord-Grey Hi, you are right, now it looks good. Thank you. wbr.

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