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

Changelog in spacemacs buffer #1070

Closed
wants to merge 9 commits into from

Conversation

CarlQLange
Copy link
Contributor

Probably not ready to merge yet, but please take a look. Requires #1065 because it uses the widget ui.

Closes #1067.

Includes a preference in .spacemacs to always show or hide the changelog on startup.

Next steps:

  1. Something along the lines of saving the changelog hash, so when it's changed after an update, we can show it automatically (if always-show-changelog is off).
  2. Actually populate the changelog file - that's @syl20bnr's job :) (though I was thinking about populating it with all the PR's that have been closed since the last release)
  3. Making it look a little nicer - I don't like having pagebreaks really, they look kinda ugly, but I'm not sure how else best to do it

@trishume
Copy link
Contributor

trishume commented Apr 4, 2015

I notice that it says :follow-link "\C-m", will RET and the mouse still work for these buttons?

@CarlQLange
Copy link
Contributor Author

They do for me. I forget why I put that in - I think the mouse wasn't working before? Must try it again with t

@tuhdo
Copy link
Contributor

tuhdo commented Apr 4, 2015

Good job!

You can give colours to the button easily by using existing faces or define a new face. Instead of plain string in :tag slot, use propertize like this:

(propertize "[Documentation]" 'face 'font-lock-function-name-face). 

We should give the top buttons the same face, a face for [Toggle Changelog] and a face for [Search in Spacemacs] to capture users' attention.

@trishume
Copy link
Contributor

trishume commented Apr 4, 2015

@syl20bnr note that this change set is a superset of #1065 and merging this will also merge that if it is not already merged.

@CarlQLange
Copy link
Contributor Author

I would sooner back #1065 for merge than this - some of the changelog stuff here is a little messy, but I think #1065 is about good to go :)

@tuhdo
Copy link
Contributor

tuhdo commented Apr 4, 2015

More to improve:

  • We can have different face for Recent Files, Projects and Bookmarks headers. Of course, you can make it easy by reusing faces.
  • Put the shortcut keys next to each header i.e. [r] Recent Files, so even first time users can guess what the key does and try it.
  • We can add some more conventional key binding: n and j to move to next entry; p and k to move to previous entries similar to TAB and S-TAB.

@tuhdo
Copy link
Contributor

tuhdo commented Apr 4, 2015

@CarlQLange The buttons do not work for me. Press RET or mouse click produces this error:

command-execute: Buffer is read-only: #<buffer spacemacs>

@CarlQLange
Copy link
Contributor Author

@tuhdo No clue why the buttons work for me but not for you - could you investigate?

@tuhdo
Copy link
Contributor

tuhdo commented Apr 4, 2015

@CarlQLange You have to enable (widget-minor-mode 1) to make the widgets work. Put the code in spacemacs-mode-hook. You can try insert those widgets into buffer without the minor mode enabled. It won't work. `. Probably you have this enable in your local repo.

@CarlQLange
Copy link
Contributor Author

I'm pretty sure I hadn't, but ok...

@tuhdo
Copy link
Contributor

tuhdo commented Apr 4, 2015

I think instead of inserting the changelog in the spacemacs buffer. better open its own window, to the right of current Spacemacs buffer.

@CarlQLange
Copy link
Contributor Author

check it out now - I changed follow-link "\C-m" to follow-link t, and enabled widget-minor-mode (though I don't think there's a difference without widget-minor-mode).

This may fix the issue @tuhdo is having, but mouse clicks no longer work for me on the links. RET does, though (it did before also)

@CarlQLange
Copy link
Contributor Author

I think instead of inserting the changelog in the spacemacs buffer. better open its own window, to the right of current Spacemacs buffer.

I dunno. That's for @syl20bnr to decide. I don't think it's a great idea really, I prefer it in the spacemacs buffer, showing only after an update.

@CarlQLange
Copy link
Contributor Author

From the docs:

To define Mouse-1 on a widget defined with define-widget, give the widget a :follow-link property. The property value should be a link action condition, as described above. For example, here is how the link widget specifies that a click shall be translated to :

(define-widget 'link 'item
   "An embedded link."
   :button-prefix 'widget-link-prefix
   :button-suffix 'widget-link-suffix
   :follow-link "\C-m"
   :help-echo "Follow the link."
   :format "%[%t%]")

This is why I used \C-m before, and it worked for me. t instead of \C-m does not make the links clickable for me anymore :-(

@tuhdo
Copy link
Contributor

tuhdo commented Apr 4, 2015

@CarlQLange But imagine after this release, we have a huge change log instead of a few ones, it would disrupt other features in Spacemacs buffer. You can try a sample change log of Emacs with C-h n. Click the button would only open in other window, so it is visible along with Spacemacs buffer but without the disruption. We should not hide the change log button and only show it after an update. It should show the changes of most recent versions, so users can choose a suitable time to view it

About this widget issue, let's revert it back to before adding the minor mode and let people try it out if the issue persists.

@CarlQLange
Copy link
Contributor Author

About this widget issue, let's revert it back to before adding the minor mode and let people try it out if the issue persists.

Reverted.

But imagine after this release, we have a huge change log instead of a few ones, it would disrupt other features in Spacemacs buffer. You can try a sample change log of Emacs with C-h n. Click the button would only open in other window, so it is visible along with Spacemacs buffer but without the disruption. We should not hide the change log button and only show it after an update. It should show the changes of most recent versions, so users can choose a suitable time to view it

The idea was that you would read through, then hide it, and then never see it again until the next update or the next time you toggled the changelog.

I guess it's six of one and half a dozen of another, I'm not strongly pushed in either direction. Not really my choice anyway, up to @syl20bnr. One way or another I would love to see another release out soon...

@tuhdo
Copy link
Contributor

tuhdo commented Apr 4, 2015

With your original commits (where you had \C-m), the mouse works for me with widget-minor-mode. I tested it with stock Spacemacs, develop branch and with only your PR commits on top. Without the minor mode, the widgets do nothing.

About the change log, it's up to @syl20bnr. But I think we should split current buffer in in other read-only buffer to the right side of current Spacemacs buffer and have a key binding to quit similar to other help buffers. Otherwise, we will have to add a key binding to jump back to the [Toggle Changelog] button and hide the change log content. To do this, we must be able to detect we are inside the change log content for the key binding to take effect.

@CarlQLange
Copy link
Contributor Author

Added that [r] Recent files: thing you mentioned above :)

We can add some more conventional key binding: n and j to move to next entry; p and k to move to previous entries similar to TAB and S-TAB.

Using k and j would break normal editing movement, and I personally hate that :) I think TAB and S-TAB do the job, and do what people expect. I expect j and k to move up and down lines, not between widgets (even if that's theoretically more useful). Don't forget that vim users would see this buffer first, think "I'll just move the cursor", and then get confused and angry and never use emacs again ;)

I'd love to use nice colourful faces for this stuff. I'd like @syl20bnr to chime in though - the buffer didn't have any colour before, and I'm wondering if it was a choice.

@syl20bnr
Copy link
Owner

syl20bnr commented Apr 5, 2015

That's cool. The change log in spacemacs will be a org file so it makes more sense to display it in its own buffer. It will be also easier to add some feature like navigation or more fancy one like checking out a specific version etc...
My ultimate goal is to make a community driven change log where contributors document their changes when they submit a PR.
Additional design info available here: #1067

@tuhdo
Copy link
Contributor

tuhdo commented Apr 5, 2015

I cloned Spacemacs again, switched to develop branch and tested it out. My Emacs on Windows didn't work. I will try with my Ubuntu machine.

UPDATE: I tested on my Ubuntu machien and it is the same. The buttons are not clickable. Good news is, I found out that there's a command called widget-button-press that can really activate the action of each button properly. We should bind RET and mouse click to that command.

@CarlQLange
Copy link
Contributor Author

It works for others, I wonder why it doesn't work for you! :) It even works for my inside a terrible term emulator...

@tuhdo
Copy link
Contributor

tuhdo commented Apr 5, 2015

Did you use Evil or holy-mode? I was using holy-mode.

@CarlQLange
Copy link
Contributor Author

Whatever's the default - I guess that's evil? I actually don't even know what holy-mode does :)

@syl20bnr Do you think this PR could be used to display the message you want to show? Not the changelog.

@tuhdo
Copy link
Contributor

tuhdo commented Apr 5, 2015

I can confirm that if I switch to holy-mode, then the buttons won't work.

  • Move your current .spacemacs elsewhere.
  • Start Spacemacs. It will ask to use whether vim or emacs (manually enable holy-mode won't reproduce this).
  • Wait for Spacemacs to complete the startup process. Then click/press on any button.

It works for you because the default command for RET in Evil is evil-ret and default mouse click is evil-mouse-drag as opposed to stock Emacs newline-and-indent and mouse-drag. So, I think in Spacemacs buffer, it's better to bind widget-button-press to both RET and left mouse click.

@tuhdo
Copy link
Contributor

tuhdo commented Apr 5, 2015

This is the description of evil-ret:

Move the cursor COUNT lines down.
If point is on a widget or a button, click on it.
In Insert state, insert a newline.

That's why you was able to press the button with RET.

@CarlQLange
Copy link
Contributor Author

Aha, brilliant! Thanks a lot! I'll fix this later on.

@tuhdo
Copy link
Contributor

tuhdo commented Apr 5, 2015

@CarlQLange @syl20bnr imo, it's better to use the functionality of [Change Log] button is [Quick Help] button. This button toggles a quick help showing the key bindings to navigate Spacemacs buffers such as TAB, S-TAB, r,p... and tell them that they can hover the mouse or move the cursor on each button to see what each button does. The current change log button simply opens the change log in a separate window.

@trishume
Copy link
Contributor

trishume commented Apr 5, 2015

@tuhdo I think a quick help button is kind of redundant. Everything those buttons can do is accessible through a documented key binding or browser URL. The reason I added those buttons was for new user discoverability, not quick access. For people that want quick access to those functions they can just use the existing key bindings. And if they really want quick navigation through the buttons they can look up how to do it in the docs.

@tuhdo
Copy link
Contributor

tuhdo commented Apr 5, 2015

@trishume but first time new users won't know about TAB or S-TAB to navigate between buttons. A short quick help like this is useful:

` Press `TAB` to move to next button.
- Press `S-TAB` to move to previous button.
- Press `SPC` (in Vim mode) or `Alt-m` (in Emacs mode) to access Spacemacs features.

Please consult Spacemacs documentation for more details.

So the new users know how to move around, especially for users who use Emacs in terminal.

@syl20bnr
Copy link
Owner

syl20bnr commented Apr 5, 2015

@CarlQLange Yes this button can be used to toggle the info message, it will be displayed by default. We can add a saved setting in .cache to toggle the message persistently. Can you add this facility ? For the label we can change it to Important notes or something like this. This will display the very important changes of the release and end with a link to the full release note (or display a buffer with the org file when it is available).

@tuhdo @trishume This discoverability issue expands to all Vim key-bindings as well, we have no mechanism to show what's available in a given state. OTOH showing all the key bindings in motion state would make the important r, b, p, tab... less visible. In the end I think the best option is @Thudo's idea, I would just make the button very tiny by labelling it [?].

@syl20bnr
Copy link
Owner

syl20bnr commented Apr 5, 2015

@CarlQLange the persistent value should be a version number, not a boolean. This way we can display the new info message when a new release is installed.

@syl20bnr
Copy link
Owner

syl20bnr commented Apr 6, 2015

@CarlQLange I pushed two functions to display some text in a frame in 63f9e72. It can be used to display the text of the toggle.

@CarlQLange
Copy link
Contributor Author

Aha, cool, yep, I'll do this stuff today. It all sounds pretty good!

@syl20bnr
Copy link
Owner

syl20bnr commented Apr 8, 2015

@CarlQLange I want to include the toggle button in the release, tell me if you have time to make it persistent as described above, if you don't have time no problem I will merge this PR and adapt it.

@CarlQLange
Copy link
Contributor Author

Sorry, my Real Job is really busy at the minute - next chance I have is Friday.

@syl20bnr
Copy link
Owner

I merged it. I will adapt it with the remarks above. We are close to the 0.101.0 release :-)

Thank you @CarlQLange 👍
Cherry-picked, merged and squashed in develop branch, you can safely delete your branch.

@syl20bnr syl20bnr closed this Apr 11, 2015
@CarlQLange
Copy link
Contributor Author

Great, I'm really sorry I didn't get the chance to work on this.

@syl20bnr
Copy link
Owner

No problem, you already did a lot 👍

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.

4 participants