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

make ENTER on a previous input field replace current input buffer #506

Closed
wants to merge 3 commits into from
Closed

make ENTER on a previous input field replace current input buffer #506

wants to merge 3 commits into from

Conversation

mspacek
Copy link
Contributor

@mspacek mspacek commented Jun 7, 2011

I found this kind of functionality really handy in pycrust/pyshell under wx. To quickly replace the current input buffer with the contents of a previous input field, just click on the previous input field and hit enter. Then you can edit it before executing it.

@mspacek
Copy link
Contributor Author

mspacek commented Jun 8, 2011

It seems that for multi line inputs (with continuation prompts), this only copies the line that the cursor is currently at. Ideally, it should copy the whole set of lines with continuation prompts. Let's see if I can get that working...

@minrk
Copy link
Member

minrk commented Jun 10, 2011

It might be helpful to just use the set_next_input machinery, used by %loadpy.

@mspacek
Copy link
Contributor Author

mspacek commented Jun 11, 2011

@minrk: I looked at set_next_input(), but I didn't see anything particularly useful about it. Setting the input buffer is easy. The harder part was deciding whether the current line is an input line, and where the multi line input starts and stops.

I'm pretty happy with it now though.

@minrk
Copy link
Member

minrk commented Jun 11, 2011

With set_next_input, all you need to do is know the prompt number for the current field - no need to try to manipulate the block, just:

  1. detect the id of the selected field
  2. execute get_ipython().set_next_input(In[id])

Also, try not to merge master into your branch. It's better to use rebase for a cleaner graph.

@mspacek
Copy link
Contributor Author

mspacek commented Jun 11, 2011

Ah, now I get it. I wasn't aware of the In and Out names in the shell. That certainly would be nicer. I'd only have to search up for the numbered prompt, instead of both up and down to find the boundaries of the input field. But in pure python mode, with no numbered prompts, I don't think that could work, could it? In doesn't seem to be bound. Also, in ipython mode, the user might for some reason bind something else to In, which would mess up retrieval of previous inputs for copying.

Sorry about merging in master. I'm still sort of winging it when it come to git. I'd be happy to create a new branch and pull request and reapply the net diff without all the messy commits.

@minrk
Copy link
Member

minrk commented Jun 11, 2011

No need to create a new PR. You can rebase -i, and squash all your commits into a neater few if you feel like things are getting cluttered.

Or, if you feel like starting from scratch, you can just clobber your GitHub qt-copy-input branch with a 'push -f' (only if you are sure), and the new commits will be associated with this PR instead of the old ones.

@mspacek
Copy link
Contributor Author

mspacek commented Jun 13, 2011

All right, I started from scratch and overwrote my previous branch. The code is a bit cleaner now too.

@@ -227,11 +227,20 @@ class IPythonWidget(FrontendWidget):
"""
text = self._control.textCursor().selection().toPlainText()
if text:
lines = map(transform_ipy_prompt, text.splitlines())
Copy link
Member

Choose a reason for hiding this comment

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

With this change, it looks like IPythonWidget's copy() method is now identical to its parent's, so it can be removed entirely and just inherit, yes?

@mspacek
Copy link
Contributor Author

mspacek commented Jun 13, 2011

Yes indeed. Good eye!

@mspacek
Copy link
Contributor Author

mspacek commented Jun 13, 2011

I just noticed that in pure Python mode, selecting a previous input field and hitting ENTER causes lockup and 100% cpu usage with no error. Not sure why. Maybe a recent commit in master?

@minrk
Copy link
Member

minrk commented Jun 14, 2011

Nope - it's your downward search never terminating (print statements inside 'while True' reveals this).

You should break if the cursor doesn't move by adding an:

if cursor.position() == end (and start in the up search) to break if it's spinning its wheels at the end of the search.

That said, searching for valid input lines does not mean they are actually input lines. I added the break test eliminating your infinite loop, and discovered that your code was picking up too much with the pure prompts. I gave the following input:

a=1
b=2
print '>>> not input'
c=3

Each as a separate cell, and if I press enter at the end of any of those cells, they will all be included (including the output of the print statement) as the next input.

@mspacek
Copy link
Contributor Author

mspacek commented Jun 14, 2011

Ah, I think this is all because there isn't a blank line in between successive inputs in pure mode like there is in ipython mode. I guess I have to limit myself to finding exactly one non-continuation input prompt (>>>), not just any number of either type of input prompt.

As for the print '>>> not input' thing, with the current approach, I don't think that's avoidable. As you say, it looks like a valid input line to the user (and to the prompt regexp), and I don't think there's any way to distinguish it from a true input line. This holds for ipython mode as well, where In [1]: print 'In [2]: not input' causes problems, but in that case I think they may be avoidable.

Perhaps a tidier approach would be to start flagging lines (or line ranges) as input fields as they're generated. I don't suppose this already happens anywhere, does it? Could this info be saved as an attrib of the widget, maybe one of the _history* attribs in HistoryConsoleWidget?

@minrk
Copy link
Member

minrk commented Jun 14, 2011

The print >>> noinput/print In[5]: foo is definitely avoidable - you can only have one non-continuation prompt in a block. The first one you see searching down means the end of the block. It doesn't, of course, protect against printing the continuation prompt.

But you are right, each input should be in an identifiable block that print statements can't mess with. The 'rich' widgets are actually HTML, I believe, so that should be quite easy, though I haven't the foggiest how to get access to that information. It should be straightforward to drop each block in a <div>, but I honestly have no idea how that works.

I also think it is perfectly appropriate for this to be a feature of the RichIPythonWidget, and thus not available in pure or plain mode (not that people actually use either, that I know of). That would make this easier, and it would also allow you to simply identify the execution ID of the current block, and actually call set_next_input(), which would be cleaner than trying to figure out the contents of the current block, etc.

@fperez
Copy link
Member

fperez commented Aug 17, 2011

Howdy, what's the status on this PR? Does it still need further work from @mspacek? I just want to make sure we complete review/merge of things before they diverge so much that the merge becomes a nightmare...

@minrk
Copy link
Member

minrk commented Aug 17, 2011

Yes, this needs a bit more work, because as it is it can actually lock up the console by not properly detecting the edges of the window, and doesn't always grab a single input field, depending on line separations.

@fperez
Copy link
Member

fperez commented Aug 17, 2011

Thanks, @minrk. So I guess @mspacek, you should let us know if you'll have a chance to finish it up. We're happy to merge it when it's completed, but the longer it stays unmerged, the higher the chances something will get moved around in master causing a conflict. So just let us know if/when you get a chance to finish it up...

@mspacek
Copy link
Contributor Author

mspacek commented Aug 17, 2011

Hello! Sorry for not getting around to this. I would like to finish this up. I haven't been keeping up with IPython developments lately however, and I'll need to remind myself of the issues involved in getting this to work. I'll try and make this cleaner by limiting the feature to RichIPythonWidget and by using the set_next_input() as minrk suggested.

@fperez
Copy link
Member

fperez commented Aug 17, 2011

Great! And no worries, it's just that we don't want your work to go unused, so it's best to wrap it up before it goes stale.

@fperez
Copy link
Member

fperez commented Nov 26, 2011

@mspacek, just curious: do you think you'll have a chance to return to this one in the near future? It still merges cleanly but the risk of a conflict goes up the longer we let it linger :) If there's anything we can do to help you out, we'd be happy to.

@mspacek
Copy link
Contributor Author

mspacek commented Nov 26, 2011

Hi Fernando,

I don't know about the near future. I'm really bogged down trying to graduate. I think the patch needs to be completely redone anyway. I am still missing the feature every time I use qt ipython, so maybe that will annoy me enough one day to finally implement it correctly. Until then, perhaps this pull request should be closed down and turned into a feature request or something?

@fperez
Copy link
Member

fperez commented Nov 26, 2011

Martin, that sounds like a good plan; you should certainly focus on your studies first now (I say that as someone who made ipython his official thesis procrastination project ;)

I've opened #1049 and linked to this PR in there, so that it's easy to find. Whenever you find the time to hack on it, you can decide whether it's easier to continue with this PR (and if so, just reopen it) or to make a clean new branch.

Thanks again for your interest, and best of luck with graduation!

@fperez fperez closed this Nov 26, 2011
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.

None yet

3 participants