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

Adapting tab mode to file content not working #59

Open
fleutot opened this issue May 28, 2021 · 7 comments
Open

Adapting tab mode to file content not working #59

fleutot opened this issue May 28, 2021 · 7 comments

Comments

@fleutot
Copy link

fleutot commented May 28, 2021

The documentation made me expect dtrt-indent to adapt indent-tabs-mode to "the right thing":

;; `indent-tabs-mode' Setting
;;
;; For determining hard vs. soft tabs, dtrt-indent counts the number of
;; lines out of the eligible lines in the fixed segment that are
;; indented using hard tabs, and the number of lines indented using
;; spaces.  If either count is significantly higher than the other count,
;; `indent-tabs-mode' will be modified.

However, opening this file /tmp/asdf.sh with basic init file ~/tmp/basic-init.el does not seem to guess and modify to the right value of indent-tabs-mode.


Test material

/tmp/asdf.sh:

#! /usr/bin/env bash

function asdf()
{
    printf "asASDFASDF"
    echo "1234"
}

printf "qwerqwerqwer\n"
result=$(asdf)

if [ df ] ; then
    if [ er ] ; then
        wefwefew
    fi
fi

basic-init.el:

(require 'package)
(setq package-enable-at-startup nil)
(add-to-list 'package-archives '("melpa" . "http://melpa.org/packages/"))
(add-to-list 'package-archives '("gnu" . "http://elpa.gnu.org/packages/"))
(add-to-list 'package-archives '("melpamilk" . "http://melpa.milkbox.net/packages/"))
(package-initialize)

(unless package-archive-contents
  (package-refresh-contents))
(unless (package-installed-p 'use-package)
  (package-install 'use-package))
(require 'use-package)

(defun my-prog-mode-hook ()
  (dtrt-indent-mode))

(use-package dtrt-indent
  :ensure t
  :config (dtrt-indent-mode t))

(require 'whitespace) ;; builtin
(global-set-key (kbd "C-c b") 'whitespace-mode)
(delete 'indentation whitespace-style) ; no warning about indent with spaces

Test result

Starting emacs with emacs -q -l ~/tmp/basic-init.el /tmp/asdf.sh, then activating whitespace mode with C-c b gives me this:
Screenshot_2021-05-28_133545

After going to the row with 8 space indent (wefwefew), deleting one space, pressing TAB, I get this:
Screenshot_2021-05-28_133934

Expected result: dtrt-indent-mode indents with spaces, since all other indentations are with spaces.


Debug data

Here is what I have found so far.

Result of dtrt-indent-diagnosis:

Guessing offset for /tmp/asdf.sh

Elapsed time for analysis: 0.001 seconds

Total relevant lines: 5 out of 17 (limit: 5000)

Histogram:

     4x   4 spaces
     1x   8 spaces

Analysis:

  offset 2 works for 100.00% of relevant lines, matching 2 distinct offsets - merged with offset 4 (0.00% deviation, limit 20.00%)
  offset 4 works for 100.00% of relevant lines, matching 2 distinct offsets - CONSIDERED
  offset 8 works for  20.00% of relevant lines, matching 1 distinct offsets - CONSIDERED
  offset 3 works for   0.00% of relevant lines, matching 0 distinct offsets - rejected: too few distinct matching offsets (1 required)
  offset 5 works for   0.00% of relevant lines, matching 0 distinct offsets - rejected: too few distinct matching offsets (1 required)
  offset 6 works for   0.00% of relevant lines, matching 0 distinct offsets - rejected: too few distinct matching offsets (1 required)
  offset 7 works for   0.00% of relevant lines, matching 0 distinct offsets - rejected: too few distinct matching offsets (1 required)

Summary:

  Best guess is offset 4 with 100.00% matching lines (80.00% required)
  Hard tab percentage: 20.00% (1 lines), -75.00% superior to soft tabs (threshold 300.00%)
  Soft tab percentage: 80.00% (4 lines), 300.00% superior to hard tabs (threshold 300.00%)

Conclusion:

  Guessed offset 4 with 100% confidence.
  Change indent-tab-setting: yes, to nil

It seems dtrt-indent-mode could correctly guess that indent-tab-setting (although there is no such variable? Is there? Wrong name? Shouldn't it be indent-tabs-mode?) should be nil.
But it could only hint the user that they probably want to set it to nil (the message seems to be a recommendation, although it is not obvious. At first glance, it looks like dtrt-indent actually made the change).

C-h v indent-tabs-mode is t, so it was not changed. According to the code comment I posted at the top, it should?

The embedded help I open from that emacs session says:

When dtrt-indent mode is enabled, the proper indentation offset
and ‘indent-tabs-mode’ will be guessed for newly opened files and
adjusted transparently.

package-list-packages tells me that what I have installed is this:

     Status: Installed in ‘dtrt-indent-20210423.745/’ (unsigned). Delete
    Version: 20210423.745
     Commit: 9714f2c5f1c9b7c21e732df8c15a870a88caba84
    Summary: Adapt to foreign indentation offsets
   Keywords: convenience files languages c 
Other versions: 20210423.745 (melpa), 20200430.1023 (melpamilk).
@jscheid
Copy link
Owner

jscheid commented May 29, 2021

@fleutot set dtrt-indent-run-after-smie and it should work.

@rrthomas when dtrt-indent doesn't run because SMIE is active (and dtrt-indent-run-after-smie is nil) there is no indication to users: dtrt-indent-diagnosis doesn't mention it, the mode-line lighter is displayed, and the minor mode is listed as active. This is clearly confusing.

The diagnosis should say something like "dtrt-indent doesn't run in this buffer because...".

As for the minor mode, I haven't been involved with its refactoring: do you think it would be better to check inside the globalized mode to decide whether or not it should be enabled for a given buffer?

@jscheid
Copy link
Owner

jscheid commented May 29, 2021

I might see if I can make time to improve this.

@rrthomas
Copy link
Collaborator

Thanks very much, @jscheid, for looking into this and other issues; also thanks @fleutot for a beautifully detailed report for this issue.

@fleutot
Copy link
Author

fleutot commented May 31, 2021

And thank you @jscheid and @rrthomas for looking into this :) If any of you wants to earn free internet pointz, here is the question I wrote on StackExchange about this.

Incidentally, and this is a little embarrassing, I didn't load the package correctly in my minimal example above either:

(use-package dtrt-indent
  :ensure t
  :config (dtrt-indent-mode t))  ; This line is NOT going to load the module by itself

I did create a custom prog mode hook, but never actually loaded it. How silly.

This works:

(defun my-prog-mode-hook ()
  (dtrt-indent-mode))

(add-hook 'prog-mode-hook 'my-prog-mode-hook)

(use-package dtrt-indent
  :ensure t
  :config
  (setq dtrt-indent-run-after-smie t)
  )

I kind of expected dtrt to replace smie altoghether. But now it looks like it rather requires it?

I suppose there is a use for dtrt-indent-mode without dtrt-indent-run-after-smie, but I don't see what it could be? If there isn't, why not make it default?

@rrthomas
Copy link
Collaborator

As far as the interaction with SMIE goes (since I introduced that) the idea is that for modes that use SMIE, dtrt-indent should defer to SMIE. If you don't use dtrt-indent-run-after-smie, then dtrt-indent will still be used in non-SMIE modes.

@fleutot
Copy link
Author

fleutot commented May 31, 2021

I don't understand "defer to SMIE". Do you mean that dtrt-mode does nothing by default if SMIE is in use? Even if dtrt-mode is active? And dtrt-indent-run-after-smie overrides that and makes it run anyway?

If that is the case, I understand @jscheid's comment better, it really is confusing having the mode active but no effect. Why not have dtrt-indent-run-after-smie on by default? Whoever activates the mode (at least locally) certainly wants it active?

@rrthomas
Copy link
Collaborator

I don't understand "defer to SMIE". Do you mean that dtrt-mode does nothing by default if SMIE is in use? Even if dtrt-mode is active? And dtrt-indent-run-after-smie overrides that and makes it run anyway?

That's right.

Why not have dtrt-indent-run-after-smie on by default? Whoever activates the mode (at least locally) certainly wants it active?

Indeed, if you're activating dtrt-indent-mode locally, then it may make sense to turn dtrt-indent-run-after-smie on. The default setting of off makes sense when you're just using dtrt-indent-global-mode. The idea is that SMIE is supposed to solve the same problem, and it's the "default" solution, so we don't want to interfere with it by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants