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

Several improvements in file local variable usage #47

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ambihelical
Copy link
Contributor

@ambihelical ambihelical commented Feb 23, 2019

The commits first to last:

  • make sure file local variables are handled properly by performing the core processing after these variables have been processed, instead of before.
  • Add ability to set related variables when any mode derived from a particular major mode is in effect
  • add new variable dtrt-indent-force-offset, which can be used to force the offset to be a particular value without affecting any of the other processing. This is intended to be used as a file local variable. This addresses Overriding tab setting for hard tabbed file #46
  • Fixed the handling of file local indent-tabs-mode setting so that it overrides dtrt-indent's analysis, which seemed to be missing from the code (dtrt-indent-explicit-tab-mode was set, but not used).

When file or directory local variables are used, they are set
after the minor mode is setup.  To fix this, use the
hack-local-variables-hook to ensure that the setup is done
after variables are set.
Apply dtrt-indent-hook-generic-mapping-list to derived modes as well.
For example, if '(prog-mode tab-width) is added to to the list,
'tab-width' will be set to the current offset for any major mode
derived from prog-mode.
Add dtrt-indent-force-offset, which when set will allow the indent to
be set to a particular value. This value is used as if dtrt-indent
guessed it as the offset value.

This is intended to be used as a file-local or dir-local variable to
set the offset value when dtrt-indent comes up with the wrong value or
can't determine the value because only hard tabs are in use in the
file. Any related values are still set (e.g. evil-shift-width).

Logging of set value will show "[forced]" when this variable is used.
A file variable setting of indent-tabs-mode should override
dtrt-indent's setting of this variable, but the code was only
partially implemented.  This commit finishes the implementation.
@rrthomas
Copy link
Collaborator

Great stuff. Is this now ready to be merged as far as you're concerned?

@ambihelical
Copy link
Contributor Author

I think so. If you are reluctant, I can give it another week of testing at my day job. I did make some minor changes during the week, and I'm not in any rush in any case.

@rrthomas
Copy link
Collaborator

Another week would be great if that's OK by you. I'll also try your branch.

@rrthomas
Copy link
Collaborator

OK to ping me in a week? Many thanks in any case!

@rrthomas
Copy link
Collaborator

I just noticed that your branch isn't working for me, in what looks like a fairly basic case: I have c-basic-offset set to 2, I visit a file that dtrt-indent evaluates (correctly) as having an indent of 4, and c-basic-offset is not adjusted. There are no file-local or directory-local overrides in this case.

Sorry, I don't have time to debug this right now, but thought you'd want to know.

@ambihelical
Copy link
Contributor Author

OK, I'll look into it.

@ambihelical
Copy link
Contributor Author

So far I'm not able to reproduce with a file with 3-space indent (no hard tabs), no .dir-locals.el, and me setting c-basic-offset to 8 in my init.el for cc-mode. It correctly detects offset of 3. What does dtrt-indent log in your case?

@rrthomas
Copy link
Collaborator

rrthomas commented Feb 27, 2019

Nothing is logged. If I run dtrt-indent-diagnosis, it's getting it right:

Guessing offset for /home/rrt/Software/smite/src/storage.c

Elapsed time for analysis: 0.005 seconds

Total relevant lines: 121 out of 269 (limit: 5000)

Histogram:

    74x   4 spaces
    43x   8 spaces
     4x  12 spaces

Analysis:

  offset 2 works for 100.00% of relevant lines, matching 3 distinct offsets - merged with offset 4 (0.00% deviation, limit 20.00%)
  offset 4 works for 100.00% of relevant lines, matching 3 distinct offsets - CONSIDERED
  offset 8 works for  35.54% of relevant lines, matching 1 distinct offsets - CONSIDERED
  offset 3 works for   3.31% of relevant lines, matching 1 distinct offsets - merged with offset 6 (0.00% deviation, limit 20.00%)
  offset 6 works for   3.31% of relevant lines, matching 1 distinct offsets - CONSIDERED
  offset 5 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: 0.00% (0 lines), -100.00% superior to soft tabs (threshold 50.00%)
  Soft tab percentage: 100.00% (121 lines), inf% superior to hard tabs (threshold 300.00%)

Conclusion:

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

However, it doesn't alter anything; maybe it thinks my setting is already 4 for some reason?

With current head, I get the same output from dtrt-indent-diagnosis, and the following logged:

Note: c-basic-offset adjusted to 4

I should also note that my c-basic-offset's global value is actually set-from-style.

@ambihelical
Copy link
Contributor Author

Very odd. I'll look at the code further when I get a chance.

@ambihelical
Copy link
Contributor Author

I don't see anything that would explain the different behaviours we are seeing. I would check that dtrt-indent-post-local-variable-setup runs for the buffer, maybe the hooks aren't working the way I thought for some reason. Also, what does your dtrt-indent setup look like?

@rrthomas
Copy link
Collaborator

rrthomas commented Mar 1, 2019

All my dtrt settings are customizations:

'(dtrt-indent-global-mode t)
'(dtrt-indent-min-hard-tab-superiority 50.0)
'(dtrt-indent-min-indent-superiority 90.0)

Checking, I find that dtrt-indent-post-local-variable-setup is added to hack-local-variables-hook. However, when I visit a file, indeed it does not appear to be run (I added a call to message to confirm this).

I tried disabling dtrt-indent-global-mode, and then manually running M-x dtrt-indent-mode after visiting a C file, but this didn't cause the hook to run either. I can confirm that dtrt-indent-mode is indeed set in the buffer in question, and after adding another call to message in dtrt-indent-mode, that that function is being run, and further that the call to add-hook is being run. But of course by then the file is already loaded, so the hook is not run…

So to summarize, if I run dtrt-indent-mode manually, after a buffer is already visiting a file, presumably this is too late, as hack-local-variables-hook has already been run. It seems that for some reason with dtrt-indent-global-mode, the hook is not being run; maybe it's too late then too?

@ambihelical
Copy link
Contributor Author

That analysis seems correct. I ran dtrt-indent-mode from a prog-mode hook, which I guess has the right timing for the code in the PR. I'll revise it so it works correctly for that case, for manual invoke and for the global mode. Not sure how to do that yet, but I guess I'll learn something in the process.

@rrthomas
Copy link
Collaborator

rrthomas commented Mar 2, 2019

Great stuff, looking forward to it!

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

Successfully merging this pull request may close these issues.

2 participants