Cleanroom re-implementations for deleted code #360
Conversation
The TODO comment said to "unload" the librrnoise library, but from looking at the usage of `ctx.librrnoise`, the only thing that needs to be done is removing that file using `removeLib()`, since the library is only *loaded* by PulseAudio/PipeWire when a corresponding module is loaded.
If the `-v` option is specified on the CLI, NoiseTorch will print logs to stderr. If not, log output will be silently discarded. Handling of this is done in `cli.go` instead of in `main.go` where the TODO was listed for it. This makes more sense, as handling of CLI options should be done as much as possible within the `doCLI` function, not in `main`.
Uses the NoiseTorch context's `paClient` and calls the `LoadModule()` function to load a module. If an error is returned, it creates a new error view (hopefully correctly, I just copied the usage of `makeErrorView()` from a different function).
kdkasad
changed the title
I copied the layout code from the setcap view, since I noticed while using NoiseTorch that the error view has the same layout, just different text. I also decided to make the title red to make the UI a little more differentiable.
Uses the same layout as the error views, only with 2 buttons instead of one. I changed the title text color to yellow to differentiate it from the message text. Other than that, it behaves the same as before.
This allows parts of the program outside the `main` function to use the app name without hard-coding a name. Will be useful to display the name in error messages.
Since I don't have an outdated version of PipeWire, I couldn't reproduce the original error message, so I just guessed as to what it should be.
|
While re-implementing the deleted UI logic, I noticed that long error messages get clipped instead of wrapping. I tried to fix this by using So until aarzilli/nucular#69 is fixed, the UI will have to clip long error messages instead of wrapping them. |
kdkasad
changed the title
I didn't notice those. I'll change that soon. |
Now uses orange as the color for the title on confirmation views since a yellow color was not already defined.
Uses the LabelWrap component instead of the Label component, which provides text wrapping capability. Instead of a small row for text and a large whitespace row, the text row is now the height of both combined. This is required as when text wraps, it needs extra space below for the text to flow onto. When the text doesn't wrap, the extra height still displays as whitespace, making the design consistent with before this change.
Turns out this was wrong and I was just using the function wrong. (See aarzilli/nucular#69 for details.) Commit 2a7ec81 implements wrapped text. This isn't technically a required part of the code re-writing and could probably stand to be its own PR, but since it's related to other changes in this PR, I'm including it here. |
TheDukeofErl
requested review from
AXDOOMER and
TheDukeofErl
and removed request for
navarro967
|
Not sure what's going on with the requesting of a review from someone resulting in a removal from a request as well but it looks like everyone is still in the requests list... |
There was a problem hiding this comment.
Hi.
Thank you for contributing to the reimplementation of problematic code in NoiseTorch. You rewrote all the code that I labeled as "must be rewritten from scratch" so far, so if you'd like to continue, I'll proceed with removing some of the Pipewire code from NoiseTorch's source code that must be rewritten. It's important that you do not look at the Pipewire code and that you have not already looked at it so that it can be a clean room reimplementation.
I've left a few (positive) comments. Two of them contain one or more questions. One requires a change. I also asked for screenshots for UI changes, I think it facilitates review.
ScreenshotsWith the old behavior, text is centered when it fits into the window entirely. When it didn't fit, the text was cut off and part of the message is completely unreadable. With the new behavior, text is left-aligned always.* When it doesn't fit in the window, the text is wrapped onto new lines so that it remains readable. *The title is still centered. Long titles will still be cut off if they don't fit in the window. OldCapabilities viewLarge window: Small window (text cuts off): Error viewLarge window: Small window: NewCapabilities viewLarge window (text is not centered): Small window (text wraps): Error viewLarge window: Small window: Edit: now I'm wondering if the "title" on the capabilities view should be a wrapped label, since it's fairly long. This does have the downside of making it left-aligned, though. |
I'm fine with the text being left aligned and wrapping. With that in mind, I do think it would be best to have consistency here: I would prefer the title, for example "Error", to also be left aligned. |
kdkasad
force-pushed
the
reimplementation
branch
from
2d49fe1 to
eded3a2
Compare
I suggest you edit the window settings of this program and enforce a minimum window size. I don't know if that's doable. This would avoid many UI glitches that happen when the window is too small. A minimum resolution could probably be 800x640? I think it would fit most modern displays. EDIT: I failed to take into account tiling window managers.
I disagree. Because it's the title, I don't think consistency applies here. I also believe that most things should be centered because it looks better (the exit status for example) as long as its not so long that it would require a line warp at the minimum window size that we would set. |
This is an option I hadn't thought of. The reason I noticed that text is cut off is because I was using a tiling WM where the window size is managed by the WM, not the application. So for those using tiling WMs, this solution wouldn't work as the minimum window size is often ignored.
I have to agree with @AXDOOMER on this one. I don't think the title on error views should be centered. I think the best solution would be one where if the window is above certain width, the text not wrap and will be centered (original behavior). Then if the window is smaller than this certain width, the text will wrap and be left-aligned (new behavior). However, I propose we defer this UI debate to a new issue and simply decide whether to keep or drop 2a7ec81 in this PR. |
Could we just shorten the title here then? I think the current one is more wordy than it needs to be: I think that something along the lines of "Missing capabilities" as a title would tell enough of the story. Potentially, we could consider adding additional explanation to the message itself, rather than the title. For example, "This program requires "CAP_SYS_RESOURCE" in order to function properly. ... ." |
That's definitely viable. I was thinking about just adding a short title above the current text and pushing the rest down. That way, we can wrap the long lines and have a centered title without much worry about it cutting off. |
I'm fine with keeping the change as is and resolving it in a new issue as well. There's a lot of ways to skin the cat in this case. |
kdkasad
force-pushed
the
reimplementation
branch
from
eded3a2 to
bf599bd
Compare
|
I think for now you could just make "Missing capabilities" the title of this window and make other longer lines warp. |
Ok, settling on this for now. Any other changes relating to text wrapping will be deferred to a new issue. |
Adds a shorter main title to the capabilities view which can be centered and not wrap. The previous title is displayed below, but is left-aligned and can wrap. Also changes "I'll fix it for you" to "We'll fix it..." because the previous sentence says "We require..." and not "I require..."
New screenshotsLarge window: Small window: For some weird reason, the row height must be >=50 in order for the text to actually wrap, even though it doesn't use all of this space and would probably fit into a height of 30. |
|
Still don't know why re-requesting a review from @AXDOOMER removes the review request for @TheDukeofErl. Re-reviews from both are welcome. |
There was a problem hiding this comment.
I think we're complete at this point. We'll wait for @TheDukeofErl 's approval before merging.
I have looked at the PipeWire code a little bit, though mostly just the logic for getting node (sink/source) names so that I could implement #358. Unfortunately, I think that probably disqualifies me for a while. |
I will @ you when ready with the instructions. |
|
awesome work, folks! |
I attempted to re-implement all of the code (outside of
ui.go) that was marked as needing re-implementation.I only used the TODO comments and what I can discover just from using NoiseTorch to figure out what the missing code blocks are supposed to do.