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

feat: added --follow option to hyprctl rollinglog #6300 #6325

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

Truenya
Copy link
Contributor

@Truenya Truenya commented Jun 3, 2024

Describe your PR, what does it fix/add?

Added --follow option to hyprctl rollinglog
#6300

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Also added onlybuild command to Makefile (it is annoying to completely rebuild whole project any time).

Is it ready for merging, or does it need work?

Ready, working, tested.

@Truenya Truenya force-pushed the main branch 2 times, most recently from f5417f9 to 17d034e Compare June 3, 2024 19:11
@vaxerski
Copy link
Member

vaxerski commented Jun 3, 2024

tbh I fail to see the use - one can always like... watch?

Maybe rollinglog should just do that?

@zjeffer
Copy link
Contributor

zjeffer commented Jun 3, 2024

Using watch is kind of annoying and very limiting: hyprctl rollinglog only shows you the last 50 lines, so using watch (with for example -n 0.1 to make sure you get every line) will keep showing the last 50 lines meaning you can't scroll up. I sometimes want to know what happened in the last 100 lines, for example, so I need the ability to scroll up in the logs.

I'll test this PR tomorrow, but from the description it does exactly what I wanted.

@Truenya
Copy link
Contributor Author

Truenya commented Jun 4, 2024

https://www.youtube.com/watch?v=JIz6l6Xlouc

@Truenya
Copy link
Contributor Author

Truenya commented Jun 4, 2024

@vaxerski can i ask for a review?

@rtgiskard
Copy link
Contributor

rtgiskard commented Jun 4, 2024

As rollinglog can not follow the output like jounalctl -f does, I'm generally using tail -f /run/user/1000/hypr/*/hyprland.log instead.

It would be good if rollinglog can follow the log, but I don't really like current implemention with keep sending log via the socket in a thread, I'd prefer a wrapper for tail which would be simple and efficient enough.

And, perhaps, use something like spdlog, and/or with syslog as backend, then just use journalctl to manage the logs.

@Truenya
Copy link
Contributor Author

Truenya commented Jun 4, 2024

I'd prefer a wrapper for tail which would be simple and efficient enough

Small detail: rollinglog works independently from any other logging methods. So we cannot use tail wrapper here. If the user does not want to write the log to a file, we cannot force him.


PS
I was also confused that I had to use features that are not used in the project(multithreading), but I did not find any other ways to provide the required functionality. I would be grateful for any hint


PSS
If we can get past the idea that there can be multiple followers, we can also use Linux FIFO, but it's basically like sockets and can't make the code more efficient. I will say more, this will slow down the execution compared to writing to the map under a lock.

@zjeffer
Copy link
Contributor

zjeffer commented Jun 4, 2024

rollinglog works independently from any other logging methods

What do you mean with this?

When I initially created the issue, my request was basically to make hyprctl rollinglog -f be a wrapper around tail -f <logfile>, without having to specify the whole path to the logfile.

If we can get past the idea that there can be multiple followers, we can also use Linux FIFO, but it's basically like sockets and can't make the code more efficient. I will say more, this will slow down the execution compared to writing to the map under a lock.

This seems to be adding lots of complexity for something that could just be a wrapper for tail, like @rtgiskard says...

@Truenya
Copy link
Contributor Author

Truenya commented Jun 4, 2024

@zjeffer You can disable logging in the configuration and still get it with the rollinglog command. The added --follow option preserves this logic.

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

additionally:

  • clang-format needed
  • no {} around short ifs (too many cases to label all)
  • wiki mr needed

Makefile Outdated Show resolved Hide resolved
hyprctl/main.cpp Show resolved Hide resolved
@Truenya
Copy link
Contributor Author

Truenya commented Jun 5, 2024

  • wiki mr needed

can u please explain more concrete? what exactly needs to be described?
Is this enough? hyprwm/hyprland-wiki#675

@Truenya
Copy link
Contributor Author

Truenya commented Jun 6, 2024

no {} around short ifs (too many cases to label all)

do you mean that it is necessary to remove {} for short Ifs? as far as I can see they are everywhere in MR

@vaxerski
Copy link
Member

vaxerski commented Jun 6, 2024

the wiki mr yea thats all, just to let people know

for ifs:

we always

if (a) 
   b();

in hyprland

@Truenya
Copy link
Contributor Author

Truenya commented Jun 6, 2024

the wiki mr yea thats all, just to let people know

for ifs:

we always

if (a) 
   b();

in hyprland

Done

src/debug/HyprCtl.cpp Outdated Show resolved Hide resolved
src/debug/HyprCtl.cpp Outdated Show resolved Hide resolved
src/debug/HyprCtl.cpp Outdated Show resolved Hide resolved
src/debug/RollingLogFollow.hpp Show resolved Hide resolved
src/debug/RollingLogFollow.hpp Outdated Show resolved Hide resolved
src/debug/RollingLogFollow.hpp Outdated Show resolved Hide resolved
src/debug/HyprCtl.cpp Outdated Show resolved Hide resolved
src/debug/HyprCtl.cpp Outdated Show resolved Hide resolved
hyprctl/main.cpp Outdated Show resolved Hide resolved
hyprctl/main.cpp Outdated Show resolved Hide resolved
@vaxerski
Copy link
Member

vaxerski commented Jun 9, 2024

this MR will clash with #6380 and since that one is smaller can you please wait for until it's merged and then just fixup this one (use log instead of std::cout for ignorable logs)

@vaxerski
Copy link
Member

vaxerski commented Jun 9, 2024

6380 is merged, can you please update the MR?

@Truenya
Copy link
Contributor Author

Truenya commented Jun 10, 2024

@vaxerski done

hyprctl/main.cpp Outdated Show resolved Hide resolved
src/debug/HyprCtl.cpp Outdated Show resolved Hide resolved
hyprctl/main.cpp Outdated Show resolved Hide resolved
@Truenya Truenya force-pushed the main branch 2 times, most recently from 8da7c1a to 9d8668e Compare June 11, 2024 10:11
@vaxerski
Copy link
Member

conflict

@Truenya
Copy link
Contributor Author

Truenya commented Jun 13, 2024

@vaxerski resolved

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

yabba dabba doo

@vaxerski vaxerski merged commit b2590b5 into hyprwm:main Jun 14, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants