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

Make truncation/replacement indicators customizable. #147

Merged
merged 30 commits into from
Aug 17, 2020

Conversation

okamsn
Copy link
Contributor

@okamsn okamsn commented Jul 29, 2020

I think it makes sense to be able to customize these settings. These are the options made:

  • selectrum-horizontal-whitespace-indicator: ".."
  • selectrum-matching-line-indicator: " -> "
  • selectrum-newline-indicator: "\\n"
  • selectrum-truncated-candidate-indicator: "..."

Should the faces applied to the strings also be customizable?

okamsn added 3 commits July 29, 2020 16:59
- selectrum-horizontal-whitespace-indicator: ".."
- selectrum-matching-line-indicator:         " -> "
- selectrum-newline-indicator:               "\\\\n"
- selectrum-truncated-candidate-indicator:   "..."
@clemera
Copy link
Collaborator

clemera commented Jul 30, 2020

I wouldn't like that this would introduce four additional user options (+ maybe faces). The scope of those is very small (multi lines aren't used by built-in commands). Maybe we could introduce one option holding a selectrum-format-multi-line function instead?

@raxod502
Copy link
Member

I like that idea.

@okamsn
Copy link
Contributor Author

okamsn commented Jul 31, 2020

I wouldn't like that this would introduce four additional user options (+ maybe faces). The scope of those is very small (multi lines aren't used by built-in commands). Maybe we could introduce one option holding a selectrum-format-multi-line function instead?

I have tried to do that by renaming the function selectrum--first-lines and adding the option. What are your thoughts?

I am unsure of the phrasing of the following function description. I feel like there is a better way of writing the final sentence.

"Default value of `selectrum-format-multi-line-function'.
Format multi-line candidates in CANDIDATES, merging them into a single line. Additionally, the matching line in a multi-line candidate is prepended to the candidate's displayed form."

@clemera
Copy link
Collaborator

clemera commented Jul 31, 2020

I have tried to do that by renaming the function selectrum--first-lines and adding the option. What are your thoughts?

Thanks, I think it would be nicer to narrow the scope of the function to the multi line strings. With the current approach user functions need to deal with the passed candidate list which means they would need to include the first cond clause and the logic to walk through the candidates in their custom functions:

...
(dolist...
    ((not (string-match "\n" cand))
      cand)
...

Another approach could be to keep the selectrum--first-lines (maybe rename it to selectrum--ensure-single-lines which seems like a better name). And then in the second cond clause we would call the new selectrum-format-multi-line-function which receives a multi line string and should return the user formatted single line string.

I feel like there is a better way of writing the final sentence.

Maybe we could cut that down to The first matching line within a candidate is prepended?

@okamsn
Copy link
Contributor Author

okamsn commented Aug 3, 2020

Another approach could be to keep the selectrum--first-lines (maybe rename it to selectrum--ensure-single-lines which seems like a better name). And then in the second cond clause we would call the new selectrum-format-multi-line-function which receives a multi line string and should return the user formatted single line string.

In that case, how would one customize the indicator for the matching line?

What if a list is used as a customizable variable, that contains the four indicators, in a way like the below diff? That way, there is only one user option created, but all indicators and their faces are still customizable.

@@ -276,6 +276,13 @@
   wrapping."
   :type 'integer)
 
+(defcustom selectrum-candidate-transformations
+  '((match      :display "->"    :face success)
+    (truncation :display "..."   :face shadow)
+    (newline    :display "\\\\n" :face warning)
+    (whitespace :display ".."    :face shadow))
+  "Transformation indicators.")
+
 ;;;; Utility functions
 
 ;;;###autoload
@@ -751,7 +758,24 @@
 (defun selectrum--first-lines (candidates)
   "Return list of single line CANDIDATES.
 Multiline canidates are merged into a single line."
-  (let ((onelines ()))
+  (let* ((onelines ())
+        (match-transformation (alist-get 'match selectrum-candidate-transformations))
+        (match-display (plist-get match-transformation :display))
+        (match-face (plist-get match-transformation :face))
+
+        (truncation-transformation (alist-get 'truncation selectrum-candidate-transformations))
+        (truncation-display (plist-get truncation-transformation :display))
+        (truncation-face (plist-get truncation-transformation :face))
+
+        (newline-transformation (alist-get 'newline selectrum-candidate-transformations))
+        (newline-display (plist-get newline-transformation :display))
+        (newline-face (plist-get newline-transformation :face))
+
+        (whitespace-transformation (alist-get 'whitespace selectrum-candidate-transformations))
+        (whitespace-display (plist-get whitespace-transformation :display))
+        (whitespace-face (plist-get whitespace-transformation :face)))
+
+
     (dolist (cand candidates (nreverse onelines))
       (push
        (cond ((not (string-match "\n" cand))
@@ -762,17 +786,19 @@
                  ;; Show first matched line.
                  (concat
                   (replace-regexp-in-string
-                   "[ \t][ \t]+" (propertize ".." 'face 'shadow)
+                   "[ \t][ \t]+"
+                   (propertize whitespace-display 'face whitespace-face)
                    (car
                     (funcall selectrum-refine-candidates-function
                              (minibuffer-contents)
                              (split-string cand "\n"))))
-                  (propertize " -> " 'face 'success)))
+                  (propertize match-display 'face match-face)))
                ;; Truncate the rest.
                (replace-regexp-in-string
-                "\n" (propertize "\\\\n" 'face 'warning)
+                "\n" (propertize newline-display 'face newline-face)
                 (replace-regexp-in-string
-                 "[ \t][ \t]+" (propertize ".." 'face 'shadow)
+                 "[ \t][ \t]+"
+                 (propertize whitespace-display 'face whitespace-face)
                  (if (< (length cand) 1000)
                      cand
                    (concat

@clemera
Copy link
Collaborator

clemera commented Aug 3, 2020

In that case, how would one customize the indicator for the matching line?

It's currently adjusted in the second cond clause which I thought to replace with the function. In this case you can do this with the function, too. But it would be another repetition so we would better pass the function two arguments where the second one is the optional first matched line:

(defun selectrum-format-multi-line (cand &optional match)
  (when match
    (concat
     (replace-regexp-in-string
      "[ \t][ \t]+" (propertize ".." 'face 'shadow)
      match)
     (propertize " -> " 'face 'success)))
  (replace-regexp-in-string
   "\n" (propertize "\\\\n" 'face 'warning)
   (replace-regexp-in-string
    "[ \t][ \t]+" (propertize ".." 'face 'shadow)
   ;; TODO: the truncation is also repetitive and should therfore also be passed as an argument
    (if (< (length cand) 1000)
        cand
      (concat
       (substring cand 0 1000)
       (propertize "..." 'face 'warning))))))

But I think your approach is a good idea, too and it would give the user less opportunity to shoot himself in the foot.

@okamsn
Copy link
Contributor Author

okamsn commented Aug 6, 2020

If it's acceptable, I would like to try just having the options as a list for now, and seeing whether that is sufficient.

One challenge I am having is figuring out the correct type for use in the defcustom, to make some parts constant and other parts changeable. The interface defcustom produces can correctly change the option, but allows for disabling elements in the alist, which is incorrect.

selectrum.el Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
selectrum.el Show resolved Hide resolved
@clemera
Copy link
Collaborator

clemera commented Aug 6, 2020

but allows for disabling elements in the alist, which is incorrect

You mean pressing the DEL button on it? I don't know if that can be restricted (I don't use customize myself), the relevant info node is (info "(elisp) Customization Types).

selectrum.el Outdated Show resolved Hide resolved
@okamsn
Copy link
Contributor Author

okamsn commented Aug 10, 2020

I decided to use the match/transformation style.

Is there any problem with using normal lists instead of plists in the customizable variable?

selectrum.el Outdated Show resolved Hide resolved
@clemera
Copy link
Collaborator

clemera commented Aug 10, 2020

Is there any problem with using normal lists instead of plists in the customizable variable?

I don't think so, as I doubt there are more customization settings added later to this option I think a normal list should be fine.

@okamsn
Copy link
Contributor Author

okamsn commented Aug 12, 2020

I've tried to improve the wording. Do you see any other places for improvement?

Copy link
Collaborator

@clemera clemera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your efforts, I think the only thing left is to improve the docs a bit more.

selectrum.el Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@raxod502
Copy link
Member

This looks great! I like how thorough the docstring is :)

@okamsn
Copy link
Contributor Author

okamsn commented Aug 15, 2020

The last sentence is changed.

@clemera clemera merged commit 771154f into radian-software:master Aug 17, 2020
@clemera
Copy link
Collaborator

clemera commented Aug 17, 2020

Thanks for your patience and great work!

@okamsn okamsn deleted the custom-indicators branch August 17, 2020 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants