penrose icon indicating copy to clipboard operation
penrose copied to clipboard

Outer gaps

Open codybloemhard opened this issue 4 years ago • 5 comments

Is your feature request related to a problem? Please describe

The gaps are not the same size all over the screen. If you have gap_px = 10, and you open two windows the gap in between is 20px. But the gap from the window to the edge of the screen is 10px.

Describe the change / addition you'd like to see made

Let the whole window have a gap. If outer_gap = gap all gaps will be uniform. If outer_gap = 0 there is no change and it will be as normal. If outer_gap > gap, it has that "in the middle' type of look.

Describe alternatives you've considered

Maybe have the gaps be aware where they are, but this seems way more difficult and unnecessary.

codybloemhard avatar Feb 24 '21 18:02 codybloemhard

https://github.com/sminez/penrose/blob/develop/docs/faq.md#are-you-accepting-contributions

This is modifying behaviour in core. The change is small, yes, but raising a PR without tests and in parallel with the issue being raised is not something I'm going to look at I'm afraid.

It looks like the current tests are passing, but I am in the middle of adding to the test suite as the current coverage is far from ideal. I have no idea if this change is going to have any knock on effects at the current time so I am closing #153 for now. Once I'm happier with test coverage, and if the change really is as small as what you have proposed then I'll be happy to include it then, provided it is tested.

sminez avatar Feb 24 '21 20:02 sminez

I believe that this change has correct behavior given the fact that all it does is shrink the effective region in the same place in the code, just like the existing bar behavior. As for tests, I did(had to) update the tests (test_screens) to include this new change. I had to include this new stuff in all tests where the existing bar behavior was present for it to compile. It seems to me that either they are both fine or both not fine.

If you want more tests, I don't know how to test it but if you come around to having more tests for the bar behavior I'm happy to make a similar test for it. By the way, I don't want this reply to come off as pressuring you to accept this change so I hope it doesn't come off as so. I'm just a bit taken back by the expectation of me adding tests given existing behavior that is almost exactly the same has no such tests either.

codybloemhard avatar Feb 24 '21 21:02 codybloemhard

The fact that the existing test coverage isn't good enough is exactly the reason why I can't accept the PR at the moment. Sorry.

sminez avatar Feb 24 '21 21:02 sminez

Just wondering-- has there been new progress on expanding test coverage? I recently discovered penrose and started tinkering with it, and I'd love to see this feature added. Is there anything I can do to help get this merged?

kwshi avatar Jul 12 '22 09:07 kwshi

Test coverage in general could do with improving still. If I'm honest, I've used Penrose as my daily driver in its current state since creating it and have never had issues. My main concern is that I'm not happy with the overall design of the internals and stalled on trying to find a cleaner way to implement things that would make testing easier and also make it easier to work on.

If you look at the branches currently up, there are a couple of false starts where I've tried to overhaul things. The changes I want / need to make are pretty broad so I wouldn't want anyone to put a lot of effort into testing / implementing new features only for me to rewrite them :disappointed:

That said, this change in particular is probably not too tricky to nail down? I'll take a look and see what I can do :eyes:

sminez avatar Jul 13 '22 05:07 sminez

Implemented as part of a full rewrite of the core of penrose which should be merging as 0.3.0 in the near future.

sminez avatar Oct 17 '22 11:10 sminez