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

Why was --msgmodule specific formatting and coloring of module names removed? #705

Closed
EvanPurkhiser opened this Issue Apr 9, 2014 · 12 comments

Comments

Projects
None yet
4 participants
@EvanPurkhiser
Contributor

EvanPurkhiser commented Apr 9, 2014

Hi!

Prior to December you used to be able to have mpv show really visually aesthetic console output using the options --msgmodule --msgcolor.

Some examples of what this looked like: 1 2.

The module names were in all capitals and aligned to the right which looked great!

Unfortunately the functionality to have the message names aligned to the right was removed in this commit. I suspect the msgmodule coloring functionality was also removed around this time.

I was curious if it would be possible to have this functionality brought back, or even an explanation for why it was removed.

@wm4

Thanks guys!

@HalosGhost

This comment has been minimized.

Show comment
Hide comment
@HalosGhost

HalosGhost Apr 9, 2014

I must say, I quite enjoy the look that these options allowed users to utliize. If such a functionality could be re-added without a great deal of clutter, I would certainly be in favor of it (for whatever that matters).

HalosGhost commented Apr 9, 2014

I must say, I quite enjoy the look that these options allowed users to utliize. If such a functionality could be re-added without a great deal of clutter, I would certainly be in favor of it (for whatever that matters).

@wm4

This comment has been minimized.

Show comment
Hide comment
@wm4

wm4 Apr 9, 2014

Contributor

So what aspect of it do you miss? That the module field is passed and aligned? That it's uppercase? The random colors (the module name was in a literally randomly picked color)?

Personally I didn't think anyone would mind, so I just changed it. I suppose it would be possible to at least readd the padding/alignment. Please no separate colors for the module name, since that would actually make the implementation more complicated.

Contributor

wm4 commented Apr 9, 2014

So what aspect of it do you miss? That the module field is passed and aligned? That it's uppercase? The random colors (the module name was in a literally randomly picked color)?

Personally I didn't think anyone would mind, so I just changed it. I suppose it would be possible to at least readd the padding/alignment. Please no separate colors for the module name, since that would actually make the implementation more complicated.

@kevmitch

This comment has been minimized.

Show comment
Hide comment
@kevmitch

kevmitch Apr 10, 2014

Member

wow, that looks awesome, I had no idea this ever existed.
both colours and alignment are nice. uppercase perhaps even a minus though. different colors for module name aren't that important.

Member

kevmitch commented Apr 10, 2014

wow, that looks awesome, I had no idea this ever existed.
both colours and alignment are nice. uppercase perhaps even a minus though. different colors for module name aren't that important.

@EvanPurkhiser

This comment has been minimized.

Show comment
Hide comment
@EvanPurkhiser

EvanPurkhiser Apr 10, 2014

Contributor

I have a small patch in the works that I'd like to propose 😄

Contributor

EvanPurkhiser commented Apr 10, 2014

I have a small patch in the works that I'd like to propose 😄

@EvanPurkhiser

This comment has been minimized.

Show comment
Hide comment
@EvanPurkhiser

EvanPurkhiser Apr 10, 2014

Contributor

So what aspect of it do you miss?

All of the above. I think the overall look was very aesthetic!

I ended up taking a look at the code to see exactly what needs to be changed and came up with a small patch for bringing back the old look.

Here's the patch

It's not perfect, the colors are still randomly selected (based on the hashed value of the module name), but I think the code is pretty simple and straightforward, hopefully not making the implementation too complicated.

I would love to get this in upstream, but wanted to bring it up here before opening a pull request. Or would it be better to open a pull request for this and move any discussion there?

Contributor

EvanPurkhiser commented Apr 10, 2014

So what aspect of it do you miss?

All of the above. I think the overall look was very aesthetic!

I ended up taking a look at the code to see exactly what needs to be changed and came up with a small patch for bringing back the old look.

Here's the patch

It's not perfect, the colors are still randomly selected (based on the hashed value of the module name), but I think the code is pretty simple and straightforward, hopefully not making the implementation too complicated.

I would love to get this in upstream, but wanted to bring it up here before opening a pull request. Or would it be better to open a pull request for this and move any discussion there?

@wm4

This comment has been minimized.

Show comment
Hide comment
@wm4

wm4 Apr 10, 2014

Contributor

All of the above. I think the overall look was very aesthetic!

Well, in my opinion, I think was pretty ugly...

Aligning the prefix is ok. I don't like uppercase for module names at all; it's ugly and complicates the implementation. Different color for the prefix... well, I don't know, this also adds a complication, and your patch doesn't restore the correct color.

Contributor

wm4 commented Apr 10, 2014

All of the above. I think the overall look was very aesthetic!

Well, in my opinion, I think was pretty ugly...

Aligning the prefix is ok. I don't like uppercase for module names at all; it's ugly and complicates the implementation. Different color for the prefix... well, I don't know, this also adds a complication, and your patch doesn't restore the correct color.

@EvanPurkhiser

This comment has been minimized.

Show comment
Hide comment
@EvanPurkhiser

EvanPurkhiser Apr 11, 2014

Contributor

Thanks for the comments on my original patch @wm4. I don't write a lot of C so it's very much appreciated.

I don't like uppercase for module names at all

I'm actually pretty indifferent on this, I changed them to uppercase in my patch since that's how it was originally. Though I couldn't find the specific commit where they were changed back to lowercase so I'm not sure what the implementation looked like previously for the uppercase conversion. I did fix my toupper call so that a unsigned char is passed by casting prefix[i]. But to be honest I'm not sure if that's the right way to do that.

I'm fine minus the uppercase, it doesn't seem like anyone is in favor of it.

your patch doesn't restore the correct color

Yeah it doesn't :( Like you had said, the implementation would be a lot more clunky to add colors back the way it was before. Since it looks like the old implementation used legacy code removed here to determine the color for each prefix. It was still completely random though since it just used the index of the module name in the module_text array.

My other idea to provide color for the prefix was to add a parameter to mp_log_new that would specify the color of the prefix for that log. It wouldn't be random then, but I don't know how to decide what each logs prefix color would be, since there isn't really a relation to color.

My argument for coloring the prefix is that it provides a nice visual separation between each modules console output, so you can easily see when one modules output stops and the other starts. So it's not so important what color the prefix is, just that they are different.

Here's my patch without the capitalization.

Thank you for taking the time to discuss this!

Contributor

EvanPurkhiser commented Apr 11, 2014

Thanks for the comments on my original patch @wm4. I don't write a lot of C so it's very much appreciated.

I don't like uppercase for module names at all

I'm actually pretty indifferent on this, I changed them to uppercase in my patch since that's how it was originally. Though I couldn't find the specific commit where they were changed back to lowercase so I'm not sure what the implementation looked like previously for the uppercase conversion. I did fix my toupper call so that a unsigned char is passed by casting prefix[i]. But to be honest I'm not sure if that's the right way to do that.

I'm fine minus the uppercase, it doesn't seem like anyone is in favor of it.

your patch doesn't restore the correct color

Yeah it doesn't :( Like you had said, the implementation would be a lot more clunky to add colors back the way it was before. Since it looks like the old implementation used legacy code removed here to determine the color for each prefix. It was still completely random though since it just used the index of the module name in the module_text array.

My other idea to provide color for the prefix was to add a parameter to mp_log_new that would specify the color of the prefix for that log. It wouldn't be random then, but I don't know how to decide what each logs prefix color would be, since there isn't really a relation to color.

My argument for coloring the prefix is that it provides a nice visual separation between each modules console output, so you can easily see when one modules output stops and the other starts. So it's not so important what color the prefix is, just that they are different.

Here's my patch without the capitalization.

Thank you for taking the time to discuss this!

@wm4

This comment has been minimized.

Show comment
Hide comment
@wm4

wm4 Apr 11, 2014

Contributor

Though I couldn't find the specific commit where they were changed back to lowercase so I'm not sure what the implementation looked like previously for the uppercase conversion.

The list of "modules" was hardcoded as list of strings, all in uppercase.

So, if nobody misses the uppercase thing, I think we're fine leaving that away.

Yeah it doesn't :( Like you had said, the implementation would be a lot more clunky to add colors back the way it was before.

No, I meant your earlier patch didn't set the correct color after printing the module prefix, as far as I understood. But your latest patch seems to fix this.

About the color itself, I don't care if it's random. You want this feature, so you should know what you want. I'm fine with it either way, as long as it affects only --msgmodule (or whatever that options is named).

Computing a hash based on the prefix string is ok, I guess. It's simple and unobtrusive.

Contributor

wm4 commented Apr 11, 2014

Though I couldn't find the specific commit where they were changed back to lowercase so I'm not sure what the implementation looked like previously for the uppercase conversion.

The list of "modules" was hardcoded as list of strings, all in uppercase.

So, if nobody misses the uppercase thing, I think we're fine leaving that away.

Yeah it doesn't :( Like you had said, the implementation would be a lot more clunky to add colors back the way it was before.

No, I meant your earlier patch didn't set the correct color after printing the module prefix, as far as I understood. But your latest patch seems to fix this.

About the color itself, I don't care if it's random. You want this feature, so you should know what you want. I'm fine with it either way, as long as it affects only --msgmodule (or whatever that options is named).

Computing a hash based on the prefix string is ok, I guess. It's simple and unobtrusive.

@wm4

This comment has been minimized.

Show comment
Hide comment
Contributor

wm4 commented Apr 11, 2014

@EvanPurkhiser

This comment has been minimized.

Show comment
Hide comment
@EvanPurkhiser

EvanPurkhiser Apr 11, 2014

Contributor

@wm4, Thanks for the comments. I've updated my patch, the changes can be seen here.

I left a couple comments with questions on my patch here https://github.com/EvanPurkhiser/mpv/commit/29095e40edc9265569e4371c5f4d7fd8214b5cc8

If you're happy with everything I can go ahead and open a PR.

Contributor

EvanPurkhiser commented Apr 11, 2014

@wm4, Thanks for the comments. I've updated my patch, the changes can be seen here.

I left a couple comments with questions on my patch here https://github.com/EvanPurkhiser/mpv/commit/29095e40edc9265569e4371c5f4d7fd8214b5cc8

If you're happy with everything I can go ahead and open a PR.

@wm4

This comment has been minimized.

Show comment
Hide comment
@wm4

wm4 Apr 12, 2014

Contributor

I can go ahead and open a PR.

Sure, go ahead.

Contributor

wm4 commented Apr 12, 2014

I can go ahead and open a PR.

Sure, go ahead.

@EvanPurkhiser

This comment has been minimized.

Show comment
Hide comment
@EvanPurkhiser

EvanPurkhiser Apr 12, 2014

Contributor

Thanks guys!

Contributor

EvanPurkhiser commented Apr 12, 2014

Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment