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

Fill gaps in dwim behavior #2995

Closed
tarsius opened this issue Feb 13, 2017 · 8 comments

Comments

@tarsius
Copy link
Member

commented Feb 13, 2017

One of Magit's most important meta-features, is that one usually has to press just one or two key to (1) show another representation/details of the thing at point in another buffer, or (2) perform some action on that thing at point.

In Magit buffers the "thing at point" is usually called the "current section", and it represents some entity or concept that is meaningful to Git. Examples of such entities include commits and files and examples of concepts include the lists (logs) of unpushed and unpulled commits.

Dwim behavior exists both for sections that represent entities as well as for those that represent concepts. For example RET on a commit in a log shows that commit in another buffer (like git show), while doing the same on the heading "Unpushed to origin/master" shows the diff for <upstream>..<current-branch>.

RET is the most important dwim command, it does something meaningful for almost all sections. But there are other commands, including, but not limited to, keys that invoke variations of this "most-dwim" command. E.g. inside a hunk C-return always jumps to the file, while RET may jump to a blob instead, depending on what the diff represents.

Knowing exactly what some section represents is another, closely related, meta-feature of Magit. This sounds rather obvious, but keep in mind that with Git, for example, a diff (and everything else) is basically just text. It is you who has to remember what command you typed to generate it, and what actions you can therefor perform on it (in Git e.g. by piping it into some other command). Magit on the other hand remembers this for you and if you invoke some command that actually performs different but closely related actions based on context, then that command automatically performs the appropriate variation.

In a sense every command (in every program) is a dwim command - if it doesn't do what you mean, than it is a command that either has a bug or that is insufficiently documented (or if you don't read documentation, a command that does not conform to your intuition).

In Magit I find it difficult to draw the line between dwim commands and other commands that just happen to do something useful and to have a mnemonic key binding. So maybe it is more useful when explaining what makes Magit different from other user interfaces, to not focus on it having many dwim commands, but to instead say that you can (1) act on everything that you can see, that you can (2) directly do (almost) everything you might wish to do, and (3) that you can do so using highly consistent and mnemonic key bindings.


To cut a long story short, there are gaps in the dwim behavior. I should have kept a list, but unfortunately I haven't. On the other hand I have in the past filled most gaps that were reported by users rather quickly, so there might not be that many after all.

To find the remaining gaps I should probably start by creating a list of all the "paths" that do make sense and then check which of those haven't actually been implemented yet.

Thinking of it, such a list would be very useful as documentation, not just to find gaps. And while a list is a good start, I should go further and create a visual guide, consisting of screenshots of the various Magit and related buffers and arrows connecting them (with associated key bindings etc). I could create a poster from that, and maybe sell it.


But for now, there's what I can find at this moment:

(Some of these might stretch the definition of "dwim behavior" a bit.)

  • Show details/other aspect in another buffer.

    • Jumping from a diff to a file/blob, is already well supported. But the opposite is not, and that is just as useful. I would say this is the biggest gap that still exists, and one of the few that have been known for a long while without getting stuffed.

      I.e. I have been planning to do something about this since long before this issue was opened: #2968 "Jump to unstaged hunk corresponding to line at point". However that still added something new, I was planning to support jumping to a dedicated diff buffer, but jumping to the diff inside the status buffer is also useful. The code presented there however is not useful.

      Actually this is already supported, but only when the file/blob buffer contains blame information and even then, this does not jump to the correct location inside the diff.

    • When blaming, allow showing the revision before the revision at point.

    • #2990 Given a commit, show log of "surrounding" commits.

    • #2949 Linkify issues and hashes in commit messages.

    • #2833 Make SPC scroll blob/file buffer while point is on diff.

  • Show/expand details in current buffer.

    • #2953 Allow selectively showing more context in diffs.
    • #2952 Show diff in file-visiting buffer.
    • #2942 Diffs should optionally support syntax highlighting.
    • #2937 Provide alternative visualizations of blame information.
  • With side-effects.

    (I should probably open a new issue "New ways of editing" for these.)

    • #2944 Make blobs editable, modify containing commit on save.
    • #2943 Edit the commit that last touched the line at point.
    • #2938 Make it possible to edit diff.
    • #2936 Keep buffers editable when blaming.
@tarsius

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

The code that calculates position offsets by taking relevant diffs into account has to be refactored. It is unreadable and therefore hard to adjust for other use cases.

This is what has prevented me from implementing #3130. Also once that is done, then magit-log-buffer-file and magit-log-trace-definition can be taught about uncommitted changes (see #3796).

@tarsius

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

The code that calculates position offsets by taking relevant diffs into account has to be refactored. It is unreadable and therefore hard to adjust for other use cases.

Turns out it wasn't that bad.

Thanks to #3860 to most glaring holes have been stuffed. And I am now going back to doing the same for new holes as they are being reported. Some linked diff-related topics listed above have not been taken care of yet, but I am closing this anyway because not closely enough related to what I had in mind when I originally opened this. The posters are not ready yet either.

@tarsius tarsius closed this May 5, 2019
@medranocalvo

This comment has been minimized.

Copy link

commented May 6, 2019

Dear @tarsius,

thank you for all these improvements.

I couldn't find the implementation of #2968 in your goto changes. Please, let me know whether I missed it. Otherwise, please find below a hacky implementation relying on your improvements:

  (defun magit-status-here ()
    "Jump to hunk corresponding to current line in magit."
    (interactive)
    (let ((filename (file-truename (buffer-file-name)))
          (line (line-number-at-pos))
          (col (current-column)))
      (call-interactively #'magit-status)
      (when filename                    ; Guard against non-file-visiting buffers.
        (let* ((filename (magit-file-relative-name filename))
               (unstaged-section (magit-get-section '((unstaged) (status))))
               (file-section (cl-find-if (lambda (subsection)
                                            (and (eq (oref subsection type) 'file)
                                                 (equal (oref subsection value) filename)))
                                          (oref unstaged-section children))))
          (when file-section
            (magit-section-show file-section)
            (let ((magit-root-section unstaged-section))
              (magit-diff--goto-position filename line col))
            (recenter))))))

The gross hack is binding magit-root-section; it would be better to generalize magit-diff--goto-position w.r.t. the root diff section.

@tarsius

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

@medranocalvo I'll do that too, maybe next week. But it's a bit more complicated than that. Going to the unstaged changes is not correct in some cases and we have to go to the staged changes instead.

@medranocalvo

This comment has been minimized.

Copy link

commented May 13, 2019

Dear @tarsius, congratulations for the excellent work and thank you again for your generosity. Let me know if I can help in any way.

@tarsius

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

👋 I have implemented this command a while ago in #3930.

@medranocalvo

This comment has been minimized.

Copy link

commented Sep 27, 2019

Dear @tarsius,
finally had the chance to test it. It works wonderfully, thank you very much!
Would it make sense to ###autoload it?
Regards,
Adrián.

@tarsius

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2019

It's autoloaded now. Seems like an oversight it wasn't autoloaded initially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Year of Magit
Goals ★★★
2 participants
You can’t perform that action at this time.