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

Fixed multiline input #186

Closed
wants to merge 1 commit into from
Closed

Fixed multiline input #186

wants to merge 1 commit into from

Conversation

dj95
Copy link

@dj95 dj95 commented Jun 8, 2016

Fixed Issue #107.

Added wm class string, so it's easier to handle lemonbar in compton, xdotool, etc.

@dj95 dj95 mentioned this pull request Jun 8, 2016

*input = 0;
while (fgets(input, sizeof input, stdin)) { }
if (!*input) {
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this, this case is already handled by parse. Moreover this way you don't clear the bar when an empty string is read.

@LemonBoy
Copy link
Owner

LemonBoy commented Jun 8, 2016

Also, the commit messages should be reworded

  • fixed LemonBoy#107 means nothing, write something descriptive like Consume all the lines before parsing
  • Add WM_CLASS ATOM for compliance compliant to what? Add WM_CLASS atom is fine here

Other than that the PR looks fine, /hopefully/ it fixes the problem @woho has.

Read input lines in non-blockin mode and parse the last one.
Should fix LemonBoy#107
@tryone144
Copy link

tryone144 commented Jun 8, 2016

Reworded the commit messages and adopted coding style.

EDIT: The WM_CLASS atom has its own PR: #187

@LemonBoy
Copy link
Owner

LemonBoy commented Jun 9, 2016

This PR is about fixing the multiline issue, open another one for the WM_CLASS thing.
Also, why there's a .diff in the changeset ?

@tryone144
Copy link

tryone144 commented Jun 9, 2016

Sorry, I've forgot to check the last commit. I will split the PR into two: multiline issue and WM_CLASS.

I haven't planned releasing this PR so soon, so the .diff is just a leftover I've forgot to delete prior the commit.

@dj95 dj95 changed the title Fixed multiline and added wm class string for compliance Fixed multiline input Jun 9, 2016
@tryone144 tryone144 mentioned this pull request Jun 9, 2016
@tryone144
Copy link

The WM_CLASS commits now have their own PR: #187

@alexd2580
Copy link

I object to merging this PR, since with these changes the same bug will happen as has been observed by @LemonBoy in a slightly different context.

Your patch seems to break my statusbar [1] for some reason

[1] http://ix.io/gTm

The reason for this breakage is that the statusbar script uses echo -n multiple times to generate one single line. Therefore the input buffer of lemonbar will be filled partially multiple times with small delays inbetween.

With a blocking fgets, this problem mentioned above does not occur, because it waits for EOS or \n which is not sent until the complete line is terminated with an echo or with the statusbar script exiting and closing the pipe.

With a non-blocking fgets (proposed in this PR) the problem occurs: after each echo -n the input buffer is drained, thus reading, parsing and displaying a partial statusbar message.

While this does improve performance and fix the {echo a; echo b} | lemonbar -p-bug, as shown here, it also introduces race conditions into lemonbar, when using it with any status generator that uses multiple statements (echo, cout) to generate one single \n-terminated line and is even a little bit more complex than date | lemonbar.

@tryone144
Copy link

This PR has been made redundant by the superior changes in 9e35bd6. Please close.

@dj95 dj95 closed this Nov 5, 2020
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.

Multiline input
4 participants