Skip to content

Conversation

alexander-yakushev
Copy link
Member

Frontend part of the pagination feature.

Keys are arbitrary, suggestions are welcome.

  • . for next page
  • , for previous page
  • z to set a new page size inside the inspector

Missing features:

  • specifying a default page size. Right now it is hardcoded to 32.

Sister pull request is clojure-emacs/cider-nrepl#238.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(interactive "nPage size: ") is probably enough (and might be slightly more appropriate).

@Malabarba
Copy link
Member

Looks good to me. (The clj side too).

For the keys, np are good candidates, and I think jk or hl will make sense to a lot of (vim) people. If we want to imitate Emacs help buffers, C-c C-b and C-c C-f are the keys used.

That said, having actual previous/next page buttons would probably be the most help to most people.

@Malabarba
Copy link
Member

I just noticed this, but docstrings would be nice too. Specially since they're user level commands.

@alexander-yakushev
Copy link
Member Author

Done. I'll wait for @bbatsov to say his word about keybindings and then change them too.

@bbatsov
Copy link
Member

bbatsov commented Jul 18, 2015

The new functionality should also be mentioned in the changelog.

Btw, how did you derive the 32 default?

@alexander-yakushev
Copy link
Member Author

Btw, how did you derive the 32 default?

By raising 2 to power of 5 ;).

@bbatsov
Copy link
Member

bbatsov commented Jul 18, 2015

specifying a default page size. Right now it is hardcoded to 32.

So the only question now is are we doing this as part of the current PR. I can imagine it'd be nice for this to be some defcustom, but there should also be some command to change the value on the fly.

@alexander-yakushev
Copy link
Member Author

There is a way to change it on the fly with z. If we want to hold the default in a variable, we have to pass it at the beginning of the inspection, I suppose.

Please also suggest a replacement for z. I don't like it personally, but it was the first thing that came into my mind thinking of siZe.

@bbatsov
Copy link
Member

bbatsov commented Jul 18, 2015

There is a way to change it on the fly with z. If we want to hold the default in a variable, we have to pass it at the beginning of the inspection, I suppose.

Yeah. The debugger does this for its object print length settings. That should be easy to do for the inspector as well.

Please also suggest a replacement for z. I don't like it personally, but it was the first thing that came into my mind thinking of siZe.

No brilliant ideas here. I'd just suggest something that's easier to press (doesn't require the use of the little finger).

@alexander-yakushev
Copy link
Member Author

s might work, and keeps it somewhat mnemonic (Size), and the binding is uncommon enough.

Done with the default page size.

@bbatsov
Copy link
Member

bbatsov commented Jul 18, 2015

I'm fine with s.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending nil to nREPL will byte you, as it gets encoded as an empty vector. :-) It's best to simply not send it at all, when nil. Here's an example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do I check for nil first, and if it is nil, I don't send that value? Sounds messy. Maybe just (or it with a hardcoded default?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what we do everywhere. It's not ideal, but you can't simply encode EL nil as a Clojure nil, as it's also an empty list in Emacs Lisp, and you can never know what exactly was it used to represent.

@alexander-yakushev
Copy link
Member Author

Added a call to or. The case is already very marginal, someone will have to deliberately change the default page size and set it to nil. I'd say let them burn then, but OK.

@bbatsov
Copy link
Member

bbatsov commented Jul 18, 2015

I'd say the Emacs part looks solid. Just get the Clojure tests passing and I'll have this merged.

@alexander-yakushev
Copy link
Member Author

They are passing. Eastwood and cljfmt cannot into modern threading macros.

bbatsov added a commit that referenced this pull request Jul 18, 2015
Add pagination of long collections to inspector
@bbatsov bbatsov merged commit b3b9bfe into clojure-emacs:master Jul 18, 2015
@bbatsov
Copy link
Member

bbatsov commented Jul 18, 2015

Victory!

@alexander-yakushev
Copy link
Member Author

🎉

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