Truncated search pattern in header #177

Walheimat · 2024-09-17T18:08:35Z

This is an attempt to tackle #164.

It's a bit awkward since it means we need to use a function for the help-echo prop that needs to access rg-cur-search to access the full search pattern.

Maybe you have a better idea.

dajva · 2024-09-19T16:45:45Z

Thanks for the PR. I like the feature, might even be something to consider to be enabled by default for some proper limit.
Let's discuss the semantics first. There are some style issues etc that we can discuss after that.

My main concern is that it's hard to discover and hit (with the mouse) the truncation string. I ran this locally and thought there was a bug since I didn't see the full search string when hovering the truncated version. Then I realized I needed to hover the three dots which is part of the same entity visually.
What I think would improve the usability is to have the truncation string as a separate entity of the header line, separated by one space instead of two to indicate that it belongs to the search string. This would make it stand out a bit more and have it's own highlighting that would make it easier to discover and hit IMO. Perhaps a bit trickier to implement though.

The other issue is a keyboard trigger. Since this is emacs we need some way to echo the full search via key binding.

I do think it should be possible to get rid of the help function but the header-line-format is tricky so I can't really give any concrete advice before testing a bit myself. See if I can get around to that.

Walheimat · 2024-09-19T20:37:36Z

Those are good points, I'll have another go at it soon, thanks.

Walheimat · 2024-09-20T18:01:48Z

So I just found truncate-string-to-width which has some interesting options:

Truncate string STR to end at column END-COLUMN.
The optional 3rd arg START-COLUMN, if non-nil, specifies the starting
column (default: zero); that means to return the characters occupying
columns START-COLUMN ... END-COLUMN of STR. Both END-COLUMN and
START-COLUMN are specified in terms of character display width in the
current buffer; see `char-width'.

Since character composition on display can produce glyphs whose
width is smaller than the sum of `char-width' values of the
composed characters, this function can produce inaccurate results
when used in such cases.

The optional 4th arg PADDING, if non-nil, specifies a padding
character (which should have a display width of 1) to add at the end
of the result if STR doesn't reach column END-COLUMN, or if END-COLUMN
comes in the middle of a character in STR. PADDING is also added at
the beginning of the result if column START-COLUMN appears in the
middle of a character in STR.

If PADDING is nil, no padding is added in these cases, so
the resulting string may be narrower than END-COLUMN.

If ELLIPSIS is non-nil, it should be a string which will replace the
end of STR (including any padding) if it extends beyond END-COLUMN,
unless the display width of STR is equal to or less than the display
width of ELLIPSIS. If it is non-nil and not a string, then ELLIPSIS
defaults to `truncate-string-ellipsis', or to three dots when it's nil.

If ELLIPSIS-TEXT-PROPERTY is non-nil, a too-long string will not
be truncated, but instead the elided parts will be covered by a
`display' text property showing the ellipsis.

Unfortunately, using ellipsis-text-property does not seem to work in the header line but I'll check if I can either get it to work anyway or build something similar.

UPDATE: So this wouldn't really help since setting display means we can no longer use help-echo and keymap etc.

Walheimat · 2024-09-20T18:41:40Z

The other issue is a keyboard trigger. Since this is emacs we need some way to echo the full search via key binding.

How would you currently do that?

For now I just pushed a change to use the built-in function and apply the help echo to then entire text.

coveralls · 2024-09-23T14:41:16Z

coverage: 83.449% (+0.3%) from 83.144%
when pulling 71df25a on Walheimat:feature/truncated-search-in-header
into e9ca15d on dajva:master.

dajva · 2024-09-23T15:04:17Z

How would you currently do that?

Not sure TBH. I was thinking there was something in the tooltip package that could be used. Perhaps that don't make sense for non mouse input. Otherwise just a message would do.

For now I just pushed a change to use the built-in function and apply the help echo to then entire text.

This is better and good enough I think.

Walheimat · 2024-09-25T16:07:46Z

So what are the style issues you'd like addressed? And what could be a good default limit?

I'll check if I can add some tests for the new functions in the meanwhile to not tank the coverage.

dajva · 2024-09-25T17:27:53Z

rg-header.el

@@ -97,6 +106,31 @@ items that the map will be applied to."
                  help-echo ,help
                  keymap ,map)))

+(defun rg-header--truncate-search-pattern (search)


Even though the -- convention is kind of nice for private stuff, I have never used it in this package so let's keep it out to be consistent.
Same for the other similar names.

dajva · 2024-09-25T18:38:14Z

rg-header.el

+  (and (numberp rg-header-max-search-string-length)
+       (< rg-header-max-search-string-length (length search))))
+
+(defun rg-header--search-help (window _object _pos)


&rest _ instead?

dajva · 2024-09-25T18:39:02Z

rg-header.el

+
+(defun rg-header--search-help (window _object _pos)
+  "Get the search help for WINDOW at POS."
+  (declare-function rg-cur-search-pattern "rg-result.el")


I'd like this to be a top level declaration, right after the require statements if possible.

dajva · 2024-09-25T18:40:48Z

rg-header.el

@@ -113,8 +147,8 @@ If FULL-COMMAND specifies if the full command line search was done."
                                        ("literal" rg-literal-face)
                                        ("regexp" rg-regexp-face))))
             (rg-header-mouse-action
-              'rg-rerun-change-query "Change search string"
-              `(:eval (rg-search-pattern ,search)))
+              'rg-rerun-change-query #'rg-header--search-help


Personally I would probably evaluate the help string here instead of using a function. I don't mind this though, so you can keep it if you like.

It would just mean that we wouldn't evaluate it lazily. I'm guessing the performance impact is negligible though.

If you prefer it that way I can change it.

dajva · 2024-09-25T19:03:10Z

test/rg-header.el-test.el

+  ;; When predicate is true.
+  (rg-unit/mock-truncation-predicate (:max 11 :predicate always)
+    (should (string=
+             "everything…"


I think you need to use the actual ellipsis in these test to ensure they pass in all envrionments. So something like:

(concat "everything" (truncate-string-ellipsis))

In my test runs ellipsis ends up being "..." (three separate dots).

Ah! Good catch.

dajva · 2024-09-26T08:54:44Z

rg-header.el

+
+;; Customization
+(defcustom rg-header-max-search-string-length nil
+  "The max line length of header line items."


This doc string seems a bit off target. Could we change this to something like: The max length of header line search string item.

dajva · 2024-09-26T09:01:11Z

And what could be a good default limit?

Not exactly sure. 40-50 seems ok to me to have it not being too much in the way.

Walheimat · 2024-09-26T16:37:11Z

I addressed all your points (I hope) in the latest commit. I will rebase everything into a single commit if you're fine with the changes now.

dajva

Looks good. Just one thing before it can be merged.

dajva · 2024-09-27T09:36:16Z

rg-header.el


 ;; Customization
-(defcustom rg-header-max-search-string-length nil
-  "The max line length of header line items."
+(defcustom rg-header-max-search-string-length 50


Sorry, for being unclear here. I was supposed communicate that we need a keyboard binding for this to be enabled by default.
So if you either add that or keep this as nil for now. I suppose binding it to v (as in verbose) could make sense in case you want to add that.

You want a binding in rg-mode-map to toggle this?

I think I'd prefer keeping it at nil and leave it at the user's discretion to set it to a value they see fit. I think the scenario where they'd run into the search pattern length being an issue is so minute that dedicating a key to truncating it seems a bit big.

One thing that we may want to consider is to call this -string-width not string-length. What do you think?

Ok, let's put it as nil then. I think I prefer length actually. string-width gives me associations to pixel width on screen rather than number of characters in a string.

Alright, pushed (never mind the commit message, I didn't rename after all, just had it ready to go). Let me know if you're fine with it now, then I'll rebase and remove draft status. Thank you.

All seems fine. So you can clean up and then I merge

This adds new variable `rg-header-max-search-string-length`. If the variable is set to a number and the search pattern exceeds that threshold the header item of the result buffer is truncated. Function `truncate-string-to-width` is used. The help echo of the item now displays the full search pattern if truncation was applied.

dajva · 2024-10-02T20:37:00Z

Thanks a lot. Merged.

Walheimat marked this pull request as draft September 17, 2024 18:16

dajva requested changes Sep 26, 2024

View reviewed changes

dajva requested changes Sep 27, 2024

View reviewed changes

Walheimat force-pushed the feature/truncated-search-in-header branch from cfc93ec to 71df25a Compare October 1, 2024 18:30

Walheimat changed the title ~~WIP: Truncated search pattern in header~~ Truncated search pattern in header Oct 2, 2024

Walheimat marked this pull request as ready for review October 2, 2024 06:06

dajva approved these changes Oct 2, 2024

View reviewed changes

dajva merged commit d727fe8 into dajva:master Oct 2, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Truncated search pattern in header #177

Truncated search pattern in header #177

Walheimat commented Sep 17, 2024

dajva commented Sep 19, 2024

Walheimat commented Sep 19, 2024

Walheimat commented Sep 20, 2024 •

edited

Loading

Walheimat commented Sep 20, 2024

coveralls commented Sep 23, 2024 •

edited

Loading

dajva commented Sep 23, 2024

Walheimat commented Sep 25, 2024

dajva Sep 25, 2024

dajva Sep 25, 2024

dajva Sep 25, 2024

dajva Sep 25, 2024

Walheimat Sep 26, 2024

dajva Sep 25, 2024

Walheimat Sep 26, 2024

dajva Sep 26, 2024

dajva commented Sep 26, 2024 •

edited

Loading

Walheimat commented Sep 26, 2024

dajva left a comment

dajva Sep 27, 2024

Walheimat Sep 27, 2024

dajva Sep 29, 2024

Walheimat Sep 30, 2024

dajva Oct 1, 2024

dajva commented Oct 2, 2024

Truncated search pattern in header #177

Truncated search pattern in header #177

Conversation

Walheimat commented Sep 17, 2024

dajva commented Sep 19, 2024

Walheimat commented Sep 19, 2024

Walheimat commented Sep 20, 2024 • edited Loading

Walheimat commented Sep 20, 2024

coveralls commented Sep 23, 2024 • edited Loading

dajva commented Sep 23, 2024

Walheimat commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dajva commented Sep 26, 2024 • edited Loading

Walheimat commented Sep 26, 2024

dajva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dajva commented Oct 2, 2024

Walheimat commented Sep 20, 2024 •

edited

Loading

coveralls commented Sep 23, 2024 •

edited

Loading

dajva commented Sep 26, 2024 •

edited

Loading