bakkeby / patches

Collection of patches for dwm, st and dmenu
284 stars 30 forks source link

barmodules status2d #29

Closed ilightwas closed 3 years ago

ilightwas commented 3 years ago

Hello, first of all let me just say that I'm amazed at your work with these patches and the dwm-flexipatch thing, amazing, amazing..

I think I found a bug with the current diff for the barmodules-awesomebar patch. Clicking on the bar is not working.

That happened after I applied some patches prior to the awesomebar one but just to be sure it wasn't some other patches before I went and applied it on a clean build. Only barmodules and then awesomebar and it still didn't work.

I'm also investigating if there are issues with dwm-barmodules-status2d-statuscmd-dwmblocks-extrastatus, my bar went away after applying it, but i'm not sure if it's configuration or wrong patching. I assume that goes instead of the status2d patch right? it seemed to include it, so idk. anyways..

ilightwas commented 3 years ago

Nevermind I saw that the click functionality was out now. My bad lol. But still not sure about the status2d with the other patches thing.

bakkeby commented 3 years ago

The dwm-barmodules-status2d-statuscmd-dwmblocks-extrastatus was an example patch intended to be applied on a bare dwm 6.2 and includes all the aforementioned modules. I tried it just now and while it compiles fine dwm I got a segmentation fault when I tried to run it, lol, so I don't know - must be some config missing or something.

I should probably remove that in favour of the individual patches. They are also quite dated and may be missing fixes that have gone into dwm-flexipatch.

The idea was to upgrade these barmodules patches at some point, but the only real change (in dwm-flexipatch) has been enabling a border for the bar to add a look-and-feel similar to that of client windows. Oh right, I guess I unified some features like the various indicators.

I guess if I do decide to upgrade these patches then I'll have to see what goes into that and what not.

Fuseteam commented 3 years ago

oh i unintentionally copied you naming scheme, how fun :joy:

ilightwas commented 3 years ago

Did some more testing and barmodules base then barmodules-status2d also doesn't work. I was hopping it was some of the extra patches with that huge one but seems to be status2d alone.

Edit; Tried with status2d from 583df305e12d7816daf56c0f91fd5d23e535c53a, got no crash but the last color applied to the bar bleeds into the tag numbers and bar title text color. Also manually applied the changes from 3b9ddf205f000cf69e02022960960327873e9a22 and same thing. So the crash issue must be some related to changes in 31286c28a3dc7528b7db7ad0e66373c232f02d03

drw_setscheme(drw, scheme[LENGTH(colors)]);
drw->scheme[ColFg] = scheme[SchemeNorm][ColFg];
drw->scheme[ColBg] = scheme[SchemeNorm][ColBg];`

After testing even the huge patch works somewhat after removing this. Its also probably what avoids the color scheme bleed but its also what crashes lol. Sorry I'm kinda new to the dwm code.

bakkeby commented 3 years ago

It's a bit weird, the patch was missing this line to dwm.c that creates the ad-hoc scheme that the status2d patch uses.

scheme[LENGTH(colors)] = drw_scm_create(drw, colors[0], 3);

I also improved the safeguards a bit (the original patch is rather sketchy, especially when it comes to incomplete tags). It will still cause dwm to exit if you pass it an invalid color code though. Let me know if you still have the color scheme bleeding issue, that's a rather old one.

ilightwas commented 3 years ago

Thank you very much, seems to be working fine now, even with all the extra bar patches(the big one) and awesomebar/alpha

scheme[LENGTH(colors)] = drw_scm_create(drw, colors[0], 3);

Only had to add the alpha. Since you are THE master of dwm patches, I want to add stacker, sticky, shiftview(not skipping clients) + your renamedscratchpad.

Can I use the ones on the suckless website or do you foresee some weird interaction? since it's not the flexipatch version.

And its not listed here. I don't want to ask for those if I can apply the suckless one since Idk how boring would be for you to add them here.

bakkeby commented 3 years ago

I don't foresee any issues using the patches from the suckless website. I only add things here if I make any direct changes to the patches that makes then different from what's on the suckless patches page.

There really shouldn't be any conflicts between these patches.

If you are in doubt then you can also look at the dwm-flexipatch code to look for integration hints, e.g. shiftview needs a minor change if using the scratchpads patch which adds "special" tags to hold scratchpad windows. https://github.com/bakkeby/dwm-flexipatch/blob/23c76d13b5376e78c268006da57d132883d5df9b/patch/shiftview.c#L5-L9

ilightwas commented 3 years ago

I see. There's only one catch I found out after a weird glitch.

The operations with NUMTAG are expecting the default NUMTAG value of 9. With your fix it still 9 but the scratchpads patch modifies it to include the length of scratchpads

#define NUMTAGS (LENGTH(tags) + LENGTH(scratchpads))

so instead of

    if (arg->i > 0) // left circular shift
        shifted.ui = (seltagset << arg->i)
           | (seltagset >> (NUMTAGS - arg->i));

should be

    if (arg->i > 0) // left circular shift
        shifted.ui = (seltagset << arg->i)
           | (seltagset >> ((NUMTAGS - LENGTH(scratchpads)) - arg->i));

and so on for right shift

Just a heads up for anyone who decides to do the same. Everything else seems fine with stacker, hopefully no more weird behavior.

Anyways, Thanks again for these great patches.

bakkeby commented 3 years ago

@ilightwas ah, you are right in that the scratchpads patch do introduce a macro named NUMTAGS

-#define TAGMASK                 ((1 << LENGTH(tags)) - 1)
+#define NUMTAGS                    (LENGTH(tags) + LENGTH(scratchpads))
+#define TAGMASK                ((1 << NUMTAGS) - 1)
+#define SPTAG(i)               ((1 << LENGTH(tags)) << (i))
+#define SPTAGMASK              (((1 << LENGTH(scratchpads))-1) << LENGTH(tags))

but this is actually something different than what is used with barmodules and it is merely a name conflict.

The NUMTAGS in this context is actually only used in one place, and that is when defining the TAGMASK macro which the patch replaces.

For compatibility I'd recommend this diff instead:

-#define TAGMASK                 ((1 << LENGTH(tags)) - 1)
+#define TOTALTAGS               (NUMTAGS + LENGTH(scratchpads))
+#define TAGMASK                 ((1 << TOTALTAGS) - 1)
+#define SPTAG(i)                ((1 << NUMTAGS) << (i))
+#define SPTAGMASK               (((1 << LENGTH(scratchpads))-1) << NUMTAGS)

The shiftviewclients patch itself uses LENGTH(tags) as per normal dwm, so I presume you copied it from dwm-flexipatch. https://github.com/bakkeby/patches/blob/d7d72a24cbd662d981e3eec60dab721f61232673/dwm/dwm-shiftviewclients-6.2.diff#L63-L64

ilightwas commented 3 years ago

@bakkeby I used the scratchpads from suckless and shiftview from dwm-flexipatch. I see , I see. This tag business with bitwise operations are a little alien to me, in the logic sense. I need to sit down sometime to study what the hell is actually happening as a whole.

bakkeby commented 3 years ago

Bitwise shifting alone is relatively easy to understand, but the logic that happens in shiftview is admittedly harder to get your head around.

Try and figure it out on your own and if you are having trouble with any of it then I'd be happy to explain it to you.