haskell-present does not follow its doc string

[geraldus]

This is what doc string of haskell-present states:

Present CODE in a popup buffer suffixed with NAME and set
SESSION as the current haskell-session.

However, there is nothing related to SESSION arg in function body [1]:

(defun haskell-present (name session code)
  "Present CODE in a popup buffer suffixed with NAME and set
SESSION as the current haskell-session."
  (let* ((name (format "*Haskell Presentation%s*" name))
         (buffer (get-buffer-create name)))
    (with-current-buffer buffer
      (haskell-presentation-mode)
      (if (boundp 'shm-display-quarantine)
          (set (make-local-variable 'shm-display-quarantine) nil))
      (let ((buffer-read-only nil))
        (erase-buffer)
        (insert (propertize "-- Hit `q' to close this window.\n\n"
                            'face
                            'font-lock-comment-face))
        (let ((point (point)))
          (insert code "\n\n")
          (font-lock-fontify-region point (point))
          (goto-char point))))
    (if (eq major-mode 'haskell-presentation-mode)
        (switch-to-buffer buffer)
      (pop-to-buffer buffer))))

However, haskell-process-do-simple-echo does session assignment, which is not documented [2]:

(haskell-session-assign
  (haskell-process-session (car state)))

Am I missing something?

[1] https://github.com/haskell/haskell-mode/blob/master/haskell-presentation-mode.el#L38-L58
[2] https://github.com/haskell/haskell-mode/blob/master/haskell-interactive-mode.el#L1119-L1120

[geraldus]

Looks like functionality of these functions got messed, look: haskell-present applies fontification to inserted code (font-lock-fontify-region point (point)). But this is not documented.
Now, haskell-process-do-simple-echo have optional MODE arg, which is neither documented nor used as well:

(defun haskell-process-do-simple-echo (line &optional mode)
  "Send LINE to the GHCi process and echo the result in some
fashion, such as printing in the minibuffer, or using
haskell-present, depending on configuration."
  (let ((process (haskell-interactive-process)))
    (haskell-process-queue-command
     process
     (make-haskell-command
      :state (list process line mode)
      :go (lambda (state)
            (haskell-process-send-string (car state) (cadr state)))
      :complete (lambda (state response)
                  ;; TODO: TBD: don't do this if
                  ;; `haskell-process-use-presentation-mode' is t.
                  (haskell-interactive-mode-echo
                   (haskell-process-session (car state))
                   (replace-regexp-in-string "\n\\'" "" response)
                   (cl-caddr state))
                  (if haskell-process-use-presentation-mode
                      (progn (haskell-present (cadr state)
                                              (haskell-process-session (car state))
                                              response)
                             (haskell-session-assign
                              (haskell-process-session (car state))))
                    (haskell-mode-message-line response)))))))

Likely, MODE should go to haskell-present function and used somehow before fontification applied.

[geraldus]

Very good, this is what rgrep shows:

• haskell-present is used only in haskell-process-do-simple-echo and in my own functions.
• haskell-process-do-simple-echo is used only in haskell-process-do-info

Here is my proposal:

• remove redundant optional MODE arg from haskell-mode-do-simple-echo
• remove redundant optional SESSION arg from haskell-present
• mention fontification in haskell-present's doc string and remove session related stuff

Additionally:

• slightly change presentation buffer name: add a whitespace separator if suffix is not nil, e.g. "*Haskell Presentation :type whatever*"
• disable structured-haskell-mode for presentation buffer instead of disabling quarantine face only.

It turns that MODE was introduced to be used with haskell-interactive-mode-echo in haskell-process-do-simple-echo. This function inserts read only text in REPL buffer above prompt and applying fontification to that text if mode was specified. Also, it looks like that SESSION is tended to be passed to this function too, because when it selects REPL buffer to insert text it need to know what session to pick. I guess SESSION arg was mistakenly introduced in haskell-present function instead of haskell-process-do-simple-echo. I have no strong opinion do we really need it, but I would prefer to remove this.

Besides I'd prefer to rename haskell-process-do-simple-echo to something like haskell-process-echo-or-present.

Also, I think it would be good to have some customisable variable, say haskell-process-fontify-output, which will indicate should fontification be applied to output of such functions as show-type, do-type, do-info, and output of haskell-doc-mode when printing to presentation buffer, echo area or a popup (which I want to introduce a bit later for inferred type signatures, FPco like) for sake of consistency.

Phew!

[lukehoersten]

Hmm definitely looks inconsistent. Thanks for the investigation. Have a PR to clean it up? @gracjan may know the history of why it's like this.

[geraldus]

Surely, a bit later though. I want to have some feedback about custom-var (and about everything I mentioned) . It is possible to make something like multi optional choice, e.g. haskell-process-fontify-output could be presented as list of symbols haskell-fontify-echo-area, haskell-fontify-presentaions, haskell-fontify-popup and presented as group of checkboxes.

BTW, assigning session to buffer (or buffer to session) will be helpful when you want to kill session and all its buffers. Currently presentation mode buffers remains opened after you hit q, in other words it is not being closed, but hidden, like dired and customisation buffers does.

[gracjan]

I really like the simplification that this brings.

Thing I'd like to have:

• name of the buffer simply ":type whatever", this is distinctive enough to be recognizable
• never fontify echo area
• fontify presentation or popup in the same way as source buffer is fontified, that is check font-lock-mode in that buffer and fontify or not fontify presentation accordingly. That will make the view consistent: same colors everywhere or no color anywhere.

The function haskell-process-echo-or-present quickly turns into haskell-process-echo-or-present-or-popup-or-something-else so we might think about more general name.

@geraldus: Is there a way for you to split this pull request? There are unquestionable things in here that we could accept right away and then proceed with new functionality in another pull request. That would be more manageable.

[gracjan]

@geraldus: after rereading everything seems to be obvious except the variable. Please do the font locking as I described above. If somebody will like to have a variable they are always free to submit another issue/pr.

[geraldus]

Ok, last question: what about renaming haskell-process-do-simple-echo?

[geraldus]

Can confirm, that haskell-session-assingassigning current buffer to session, so it is good to assign all presentation buffers to current session (thus all of them will be killed with session if asked).

[gracjan]

Rename confirmed. Use your name. Use the assign so that it is killed properly.