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

Implements #277 + minor stuff and fixes #352

Closed
wants to merge 10 commits into from
Closed

Implements #277 + minor stuff and fixes #352

wants to merge 10 commits into from

Conversation

drrmmng
Copy link

@drrmmng drrmmng commented Mar 28, 2019

This pull request contains the following:

  • Implementation for Support for PulseAudio sources in the sound block #277 through a "direction" field in the sound block. This field can be either "input" or "output" and defines whether the block is used as a sink/output/sound or source/input/microphone.

  • Added support for Font Awesome 5 Icons. The icons.name field now accepts "awesome 5" as a value.

  • Added microphone icons.

  • Fixed a few icon bugs in the sound block.

  • Fixed the icons in the xrandr block.

Things that require testing:

  • Do the Material icons work correctly? I couldn't get the to work correctly even without this PR.

  • Does the sound block as input work correctly with alsa? I wasn't able to test this.

@atheriel
Copy link
Collaborator

Thanks, this looks like impressive work. I will try to test out the ALSA stuff sometime this week.

Is the FontAwesome 5 stuff integral to these changes or can it become its own PR? Do you think it will resolve issues like #130?

@drrmmng
Copy link
Author

drrmmng commented Mar 28, 2019

Thanks!
Yea, the FontAwesome 5 stuff should probably be a separate PR. Unfortunately I don't know how to separate them... 😀

Regarding #130: I don't think this is related, I just fixed some icons that use different Unicode IDs in FontAwesome 5.

@drrmmng
Copy link
Author

drrmmng commented Apr 7, 2019

Thanks, this looks like impressive work. I will try to test out the ALSA stuff sometime this week.

Did you test it already? :)

@atheriel
Copy link
Collaborator

Unfortunately, I am not going to be able to merge this in its current form. Couple things to note:

First, you started this branch from a commit nearly six months old. As a result, there are a bunch of legitimate merge conflicts, and the branch cannot be easily rebased against master, either. Second, the commits each make a whole bunch of unrelated changes at the same time, often overwriting previous work. This makes it very, very hard for me to assess the changes. Finally, there are a bunch of unnecessary formatting modifications that make resolving the merge conflicts very difficult.

I spent a little while trying to parcel your changes out from the individual commits, but it was too time intensive, and eventually I gave up. I'm very sorry to turn away legitimate contributions, but I don't have the bandwidth to restructure your work from this branch. My suggestion would be

  1. Break unrelated changes into individual pull requests. The FontAwesome 5, xrandr, and sound icon fixes would all be welcome, but not all at once.

  2. Don't bundle unrelated edits (e.g. reformatting existing code in other parts of the file) with bug fixes or improvements.

  3. Try to write (or, more likely, rebase until you get) a "clean" git commit history so that I can compartmentalize your changes and understand them better.

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