-
Notifications
You must be signed in to change notification settings - Fork 119
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
skins and skins-qt: Improved Winamp Skins support #160
base: master
Are you sure you want to change the base?
Conversation
This PR aims to improve the Winamp Skin support for both the GTK and Qt mode, the following was done: - Improved visualizer accuracy to match behavior with Winamp where possible - Leading Zeroes by default - Clutter bar/Menu row is always visible and shows what is currently active (Doublesize mode, Always-On-Top) - Changed Clutter bar/Menu row status text to more match what Winamp shows - Songticker no longer shows --- - Visualizer no longer clears with almost every playback related action - Visualizer can now cycle through the modes by clicking on it (It doesn't work in the GTK version, but to be honest, I don't have a clue how GTK works...)
Even with it commented in mainwin_create (), the song ticker still appears blank
Thanks for working on this. 👍 It is too late though for Audacious 4.4 to get this merged. I could not find the time yet to review this in more detail. But please
|
Yeah that's fair, hopefully it'll take me a few weeks at most, is there anywhere I can see a guide on the coding style in use? |
This seems like a lot of changes that are not all objectively "improvements" but may come down to personal preference.
How was it "inaccurate" before? I did a lot of work on the visualizers years ago and like the current behavior.
No. The current default (no leading zeroes) is intentional.
IMO the "clutterbar" should be disabled by defaults. I am okay with it being optional. It may be popular with long-time Winamp fans but is not good UI for several reasons and will be quite confusing to new users.
Cloning Winamp exactly is not our goal. What exactly did you change and why do you prefer it?
What do you mean by the "songticker" and why is this an improvement?
I was not aware that it did "clear with almost every playback related action". This should be its own PR with a clearer description of the current behavior and what you changed.
IMO this should also be disabled by default. It is unusual/surprising UI behavior, at least if you're not a longterm Winamp user. |
The frame interval is defined in libaudcore/vis-runner.cc:
I am not sure if changing it will break any assumptions elsewhere in the code. It has been 30fps for a long time now. |
Not really the reply I had expected...
This is something I'll address later as I think it's important that it's not objective and contains a tiny bit of a personal preference.¹
The issue mainly came down to how the analyzer and oscilloscope behaved. The oscilloscope was more of improving it's rendering with the Lines and Solid modes (as well as fixing the scope of displaying the wrong color for one line.), as well as avoiding showing a gradient that can somewhat be seen with the Lines mode, and is very obvious with the Solid mode.
I'll come back to this too.²
Agree to disagree, it's a core part of Winamp and hiding it by default not only leaves a void behind, but also may not encourage users to click on it. If nothing's there, why have any incentive to interact with the empty space?
The info text that shows up in the "Songticker"/Marquee. I think we can sit around and discuss this for ages, so I'm more open to change it, or revert the text back to what it was. It served to make things a bit more familiar.
The "songticker" is the marque, and whilst I admit that this was personal preference, stars separating the title looks better, IMO. Dashes are a bit... intimidating, in a way.
Maybe, I think it makes more sense here since it was on my radar to improve as well as the other things the PR addresses.
How is this "unusual/surprising UI behavior"? Many times did I find myself trying to click on the visualizer box, only to find that it wouldn't change, and I tend to flip flop between either the analyzer or oscilloscope, and going into the preferences (and keeping it open) is cumbersome. ¹²With this reply, I am getting the impression that Audacious doesn't want to present itself as a Winamp alternative for Linux/Windows (anymore), and if that's the idea then that's okay. Were these changes also tested? I originally wanted to re-work this PR as per the suggestions by radioactiveman, but I'm going to hold off on doing it for now until there's a more clearer answer as to what is wanted. |
Well, what reply did you expect? You are a new contributor to the project, asking to undo changes that were made intentionally, in many cases years ago, by others who have been involved much longer.
Did you confuse Audacious with some other project? Check the description on our website: we do not present Audacious as "a Winamp alternative". Audacious is an independent audio player, standing on its own merits, which happens to be able to make use of Winamp skins. As long as I have been involved (15 years, give or take), cloning Winamp has never been the goal. I do appreciate you taking the time to describe your changes and your rationale more. And I did read through the whole thing, but unfortunately I don't have time right now to comment on every individual change you would like to make. So I will address just one example:
"There's a reason Winamp does X" means nothing to me. Again, in this project we do not things just because Winamp does them that way. For this specific case, the analyzer shows both amplitude and pitch as logarithmic because that's a better model of how the human ear & brain perceive sound. If Winamp does not do it that way, then I consider our approach better. For years, I was the most active contributor to Audacious, and much of the code and design therefore reflects my ideas and preferences for how an audio player ought to work. Of course I do not "own" Audacious, and many other people have also been involved and will have different preferences. But where a difference of opinion exists, I think that those who have invested more in the project should have more say. @radioactiveman can make a final call, but my vote on most of these changes is "no". If there are individual changes that you have a good reason to view as objective improvements (not just personal preference or because "Winamp does it that way"), then please submit those as separate PRs with rationale given. I did not test the changes since (1) I do not agree with them based on the description and (2) a bulk PR, with this many behavioral changes lumped together, is not acceptable to me. |
This PR aims to improve the Winamp Skin support for both the GTK and Qt mode, the following was done:
Edit: I forgot to mention this, but I was looking to enable the visualizer to run at 60fps but couldn't find a way, are there any pointers for this?