minad / org-modern

:unicorn: Modern Org Style
GNU General Public License v3.0
1.51k stars 46 forks source link

New Progress Bars #200

Closed jdtsmith closed 4 months ago

jdtsmith commented 4 months ago

This implements a new progress bar, drawing on our work in #196 and your other more recent version (ecb590e). It uses the draw algorithm from the former, and the updated styling, matcher regexp, faces, etc. from the latter. It is a simple, fast, pixel-perfect progress bar, with a fixed overall bar width. It uses specified space (on the [ and ] chars) and the bare minimum of allocations from replacing string displays:

The stats text can be edited, visited with point, copied, etc.

I also added a parent face org-modern-progress. This is needed because it is important for all face settings that alter font (:height, :weight, etc.) to be set consistently across both complete and incomplete. I made that parent face inherit from fixed-pitch (also important for consistent bar width). I also made it bold and reduced the height, and slightly lightened the incomplete face to avoid conflicts with somewhat less dark theme backgrounds. Here's how it looks:

image

The one price for a pixel-perfect completion bar is that the start of the stats text moves slowly by <1 char width, but i) the numbers themselves will move too as they change size (9 -> 10), and ii) bars are usually not lined up on top of each other so alignment isn't a top concern.

Another PR can consider overall alignment of progress bars and tags within the window. Let me know if you are still encountering any bar movement on item checkoff.

jdtsmith commented 4 months ago

Update: I missed the box styling (bars get really tall on tall lines), so inherit now again from org-modern-label.

image
minad commented 4 months ago

Thanks! I tried it. Now the problem is that the progress bar takes the full line height, and doesn't have a border like the other boxy org-modern-labels. I suspect the problem is that org-modern-progress inherits from fixed-pitch instead of org-modern-label. I tried replacing fixed-pitch with org-modern-label which restores the boxy design, but breaks the width and centering of the progress bars.

I am going to close this for now. It is a little too much back and forth and the result isn't satisfactory yet. I know you've put a lot of time into this, which I appreciate, but it is better to take a break here. The version we have in the main branch is already quite good and a result of our discussion process.

I won't have much keyboard time for the next three weeks, so I propose the following:

  1. We will wait over the course of a month for feedback regarding the new progress bar (as it is part of the main branch). I want to hear if the feature is generally liked or disliked. The feature in the main branch is a satisfactory approximation in my opinion.
  2. Depending on user feedback we proceed.
  3. After a month, we can come back to this and potentially add your improvements, if they work. A fine granularity and your other improvements would certainly be nice to have.
  4. I suggest you continue to test your changes for a bit longer. Maybe install your fork in your config, use it for a while, such that all issues are sorted out. Try various settings of line-spacing and compare with the status quo in the main branch from time to time. Maybe you also find other small isolated improvements that we could merge?

I hope you agree to this game plan, even if it is likely a little disappointing that we cannot proceed more quickly.

minad commented 4 months ago

Update: I missed the box styling (bars get really tall on tall lines), so inherit now again from org-modern-label.

Oh, now I wrote this elaborate rejection and you quickly fixed this in the meantime.

jdtsmith commented 4 months ago

I was following your lead from https://github.com/minad/org-modern/commit/ecb590e24d25e59bf84a63a34c29158c55f457eb, thought perhaps you had decided you liked the tall bars afterall ;).

minad commented 4 months ago

Okay, so I tried it once more and it still doesn't work. I don't know what is wrong. Either your fix doesn't work or I didn't test it correctly. (In https://github.com/minad/org-modern/commit/ecb590e24d25e59bf84a63a34c29158c55f457eb I didn't use tall progress bars, so maybe there was a misunderstanding).

In any case, we will go according to my proposal in https://github.com/minad/org-modern/pull/200#issuecomment-2113240730. I hope you understand. In my experience it is better to cycle between projects instead of trying to push one thing too hard.

jdtsmith commented 4 months ago

No reason it shouldn't work. Probably the org-modern-(in)complete faces didn't get redefined (annoyingly, compiling a file leaves defined faces alone). Please give it another test after recompiling or maybe re-evaluating the faces, and M-x org-mode to be sure all the faces are set correctly. You didn't inherit from label in https://github.com/minad/org-modern/commit/ecb590e24d25e59bf84a63a34c29158c55f457eb, hence when I translated that style: tall bars. In any case that's fixed. The two versions are actually very similar. I just apply face and display a bit more carefully for appearance and performance.

Main still has bar length problems due to inconsistent faces, btw:

image

I think this is absolutely ready to go, and it's been optimized to the bone. I understand if you still want to set this aside and test for a while; if so, in the meantime, I'd appreciate you leaving this PR open.

minad commented 4 months ago

No, unfortunately it does not work. I tried again, clean reinstall. The progress bar don't take the full height anymore, but the label is not centered and the progress bar width is not constant. Exactly as I had described in https://github.com/minad/org-modern/pull/200#issuecomment-2113240730. Let's stop here for now.

minad commented 4 months ago

Main still has bar length problems due to inconsistent faces, btw:

I don't see this. There must be a subtle difference in our setups. In contrast, your version has length problems for me. 🤯

jdtsmith commented 4 months ago

Mine works for me, yours works for you, pretty funny. Do you see something similar on mine as I see on yours? I did formerly have some slight bar length differences on my end because org's faces were getting mixed in. Now (as of this PR) I just overwrite them, which fixed it.

minad commented 4 months ago

Mine works for me, yours works for you, pretty funny. Do you see something similar on mine as I see on yours?

There was indeed a mistake in my code. Fixed in https://github.com/minad/org-modern/commit/ea829ce4b3ac035808d411b1a088e0947d75f554. I also added a few more examples for testing. The additional, subtle length differences you're observing are likely a result of not using a fixed pitch font.

Now (as of this PR) I just overwrite them, which fixed it.

Makes sense.

jdtsmith commented 4 months ago

That fixed it in main on my end (well, the first 3 bars are still blank ;):

image

likely a result of not using a fixed pitch font

I do use fixed-pitch fonts exclusively. I think it could be because bold weight may have different width and faces were not uniform.

When you have the bandwidth again, I'd appreciate you helping debug the bar length issues you are still seeing in this PR (which mystify me). As you can see from the screenshots, I have perfect bar length uniformity, so not sure what could be the problem.

minad commented 4 months ago

When you have the bandwidth again, I'd appreciate you helping debug the bar length issues you are still seeing in this PR (which mystify me). As you can see from the screenshots, I have perfect bar length uniformity, so not sure what could be the problem.

I think the problem may be specific to my setup, since I use a variable pitch (or almost fixed width) font for org-modern-label (Iosevka Aile). I like this font for the labels, since it gives them a slightly distinct look, such that they stand out from the underlying fixed pitch text.

So it could be that everything is alright in your PR technically. Still, with such a configuration, your formatting is actually worse than my simplistic one from the main branch. Note that many people use Org with variable pitch fonts, also for the text, but except tables. So it is not an uncommon setup.

To be clear, I will revisit this, when I have the bandwidth again. The points you make are appealing (granularity, allocations).

jdtsmith commented 4 months ago

The fixed-pitch in the org-modern-label inheritance path should override that, so it should be fine as long as your fixed-pitch is indeed fixed. I do find Emacs weirdly remembers inherited faces and sometimes requires re-evaluating them, especially when going back and forth between two version.

I've tested this PR with a variable width font family for org-modern-label, and it works just fine, correctly overriding with my fixed font. Oddly enough, however, the current main does not handle a variable width label font well. Since it doesn't inherit fixed-pitch, you get bar length problems again:

image

🤯 indeed.