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

format-entry widths #67

Closed
bdarcus opened this issue Apr 10, 2021 · 7 comments · Fixed by #84
Closed

format-entry widths #67

bdarcus opened this issue Apr 10, 2021 · 7 comments · Fixed by #84
Labels
bug Something isn't working
Milestone

Comments

@bdarcus
Copy link
Contributor

bdarcus commented Apr 10, 2021

#66 fixed one problem, but introduced another minor one.

The default suffix template has ${tags:*}, which should extend the last column to the end.

But it's truncated much earlier (if you see below, it seems like the tags are truncated at about 27 characters).

Screenshot from 2021-04-13 12-41-19

Must be because we no longer assume a single template.

Here's what I see in ielm:

ELISP> (bibtex-actions--process-display-formats bibtex-actions-template-suffix)
((t "          ${=key=:15}    ${=type=:12}    ${tags:*}" . 45))
ELISP> bibtex-completion-display-formats-internal
((t "${author:36} ${title:*} ${year:4} ${=has-pdf=:1}${=has-note=:1} ${=type=:7}" . 53))
@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 14, 2021

Can you two take a look at this @jethrokuan @nobiot?

If you use multiple templates in org-roam, it will have the same problem.

Also, note: I made some recent performance optimizations on this code that may also be relevant for org-roam templating.

@nobiot
Copy link

nobiot commented Apr 14, 2021

Is this "bug" relevant for the latest commit available from MELPA? Sorry I am a bit confused about how to contribute here.

By the looks of the current source code, these lines use the templates to produce a long string of text for each candidate.
https://github.com/bdarcus/bibtex-actions/blob/main/bibtex-actions.el#L143-L152

If the suffix has issue of truncating too early, then it should be the issue of the function bibtex-actions--format-entry not correctly processing ${tags:*}" for suffix -- perhaps the asterisk is not properly interpreted? Char length 27 appears to be an outcome of 15 + 12, ignoring "*".

I could look at the runtime in edebug if I know which commit/branch I can reproduce the issue with.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 14, 2021

It's apparent in the latest MELPA release/main branch, and was introduced a few days ago.

For context, I forked the bibtex-completion-format-entry function, and did not touch the logic of it.

So like you, I think that's where the problem is.

I'm assuming the issue is something to do with the distinction between the total width for the intended table, and the widths for each template. But not that it doesn't recognize the asterisk (I think the 0 at the end is that).

TIA!

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 14, 2021

As in, it works as expected if there is only one template/section.

Bibtex-completion and its ivy and helm frontends were designed around that assumption.

Maybe I need an optional "table-width" parameter on format-entry, and then somehow incorporate that into the logic?

@nobiot
Copy link

nobiot commented Apr 14, 2021

I'd debug focusing on this part:

       (truncate-string-to-width
        field-value
        (if (> field-width 0)
            field-width
          (- width (cddr format)))

The width var is calculated in the calling function; 34% of the frame width. If * is translated to zero, perhaps (- width (cddr format) yields an unexpected result.

Can't debug now (working) but I'm curious ;)

I'm assuming that having 2 templates is not an issue -- it may be, but it does not look to be the case so far. I might well be wrong on that, of course.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 14, 2021

Interestingly, if I change the widths like so, the rendering is closer to expectation.

         (main-width (truncate (* (frame-width) 0.65)))
         (suffix-width (truncate (* (frame-width) 0.55))))

That clearly doesn't add up, because the sum is then greater than the frame-width, but the string does not extend beyond the frame.

That's why I'm thinking there's a math issue.

Aside: I added those lines because earlier the table was extending beyond the frame, which caused rendering problems in some completion systems.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 14, 2021

I opened a branch on #87 to figure this out.

But yes, I think you've identified the offending section, and more specifically it's probably this line:

   (- width (cddr format)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants