PY-10985/IDEA-104596: Prefix based console history #593

fitermay · 2017-06-19T02:07:51Z

Some notes:

Not sure why old controller is synchronized everywhere as most methods cannot possibly be invoked off the EDT. I made the 'MasterController' class thread safe in case loadHistory is invoked off the EDT. This never happens inside the idea-community codebase but maybe some other IntelliJ based IDEs do it.
Not quite sure what the multiline flag is supposed to do. It's not ever set inside this codebase but maybe other IDEs use it. For this reason tried to modify as little of existing code as possible and preserve existing behavior.
Added PCollections dependency to be able to efficiently snapshot state. It's a 24KB jar, so hopefully not a big deal
The behavior is based on QtConsole, so where it behaves the same it's probably intentional, where not is probably a bug.
It may make sense to move some classes from lang-impl to lang-api.

fitermay · 2017-06-20T16:12:13Z

Elizaveta239 · 2017-08-11T16:40:59Z

Hi Yuli!
Thank you for your pull request!
Could you please tell me why you're using pcollections? We're trying to add new libraries to the project if and only if it's really necessary, but this library looks like something you can replace with standard collections (if it isn't true, please, explain, why). I haven't found any performance benchmarks, but authors might have some problems with it: hrldcpr/pcollections#4. Also the latest commit to this library was a year ago, it doesn't look like a reliable project.

fitermay · 2017-08-12T13:07:09Z

@Elizaveta239 My main motivation is not really performance -- there are really no CPU intensive tasks here. I'm using pcollections to make it simplify reasoning about concurrency in places where state may be updated by multiple threads/consoles. Also to avoid placing unnecessary locks or making defensive copies.

I thought this library is ok to include because the kotlin guys find it reliable enough to implement kotlin persistent collections based on it: https://github.com/Kotlin/kotlinx.collections.immutable. Also the jar is tiny (under 40KB).

Could you ask Ilya Gorbunov( @ilya-g ) if the library is reliable enough? I assume he knows more about it than we do. If it's not reliable or we can't include it for some reason I can re-implement without it.

Also please let me know if you find any other issues. Thanks!

Elizaveta239 · 2017-08-15T22:18:41Z

@fitermay If so, why don't you want to use this Kotlin library?
As I've already said, we can't rely on pcollections library authors. And when we merge it to the project, we will have to support it.

During the next two weeks I will be on vacation, and after that I'll finish reviewing.

fitermay · 2017-08-15T23:54:05Z

@Elizaveta239 I'd rather use the Kotlin library. Are you ok with using it? Not sure of the right way to add a kotlin lib to the project. Do I just drop the jar into lib?

fitermay · 2017-08-23T00:28:57Z

@Elizaveta239 Replaced pcollections with kotlix.immutable, fixed issue caused by kotlin compiler bug and rebased onto latest master.

ilya-g · 2017-08-30T06:56:18Z

Currently the kotlinx.collections.immutable library doesn't offer the performance better than pcollections, as it is built on top of pcollections. That library was chosen as the implementation not because of its outstanding reliability, but because it's just easier to adapt it to the proposed API and the library itself is rather small.

@fitermay Regarding the usage of the immutable persistent list in this PR, I don't see a big need in that, so that it could justify adding another library to the IDEA codebase. You can use any read-only List to hold entries collection, or if there is guava library available, you can consider using the immutable list (not persistent) from there.

ilya-g · 2017-08-30T06:58:43Z

platform/lang-impl/src/com/intellij/execution/console/PrefixHistoryModel.kt

+
+
+  override fun addToHistory(statement: String?) {
+    val stmt = statement ?: return


You can just check if (statement == null) return and it will be smart-casted further. It's more clear and there's no need in an additional local val.

fitermay · 2017-08-30T07:03:17Z

@ilya-g I could use a read only list but then I would have to make a copy each time I need to mutate state. This isn't a big deal IMO because the lists are small but there was some discussion about performance so I don't know if that will be accepted.

fitermay · 2017-08-30T07:51:07Z

Anyways I will rewrite with regular mutable collections. Most of the operations here must be on the EDT anyways. No idea why the original model used those unnecessary locks but it had me in the beginning.

Elizaveta239 · 2017-08-30T09:55:46Z

Hi @fitermay! I'm back and I'm ready to continue a review.

I like the idea to use regular collections.
But I have an another question about one of your decisions. The state in PrefixHistoryModel is being updated only after user's actions with history, so it doesn't happen very often. Why are you using atomic operations instead of synchronized code? Which advantages does it have in this case?

I haven't passed a build to our QA team yet, but I've met a strange behaviour after a short testing.

Execute a multiline command, for example:

if True:
    print("Hello")

Press "up" and get it from the history. The caret is in the beginning of the command.
If you press "down" twice, a caret goes down and after that the command is gone - it's ok, we're returned to the current command.
But if on the step 2 you press "right" twice and only after that press "down" twice, the command doesn't disappear, you can't leave history in this case. Even if you move caret to the beginning of the last (empty) line, you can't leave this command. Please, fix it.

If in the state 4 you decide to delete the command and just press enter, it doesn't execute empty command, but it prints "..." for entering a multiline command. It looks like a bug as well.

As far as I understand, in the current implementation you assume that the prefix for a history search is a substring of the first line from the beginning to the caret position. It works fine when you navigate through the history to the older commands (the caret is always on the first line). But if you decide to go back to the most recent command (press "down" many times), the console should show you only commands, which you've already seen. And your caret position on the first line shouldn't affect the commands order. I mean if I searched for commands with if True: prefix, when I go back to my command, even if my caret in the beginning of the line, I should see only commands with if True: prefix.
It was really surprising for me to find some new commands in a history and it took some time to realize that it depends on the caret position. Am I right?

Elizaveta239 · 2017-08-30T10:05:07Z

platform/lang-impl/src/com/intellij/execution/console/PrefixHistoryModel.kt

+      }
+      newEntries += stmt
+
+      return@updateAtomically State(newEntries, newEntries.size)


Minor: here and in some other places you can remove return, since it's a last statement in a lambda and it will be implicitly returned.

fitermay · 2017-08-30T11:45:58Z

@Elizaveta239 Yes, it depends on the caret position on the first line. Like I said I copied the behavior of QtConsole. I think the behavior you want is more intuitive though. I will fix it.

However, please do experiment with QtConsole and let me know which behavior you want.

Elizaveta239 · 2017-09-05T10:45:06Z

@fitermay Sorry, I missed your comment editing. Yes, I think, QtConsole isn't very intuitive. If you have time, it would be great if you implement another behaviour. Please, let me know if you're not going to change it - I'll do it on my own later.

Replace kotlinx immutable with regular collections Change navigation to previous to be previous seen entries regardless of prefix

fitermay · 2017-09-06T01:17:02Z

@Elizaveta239 Hi, Didn't have time to work in it until today. I've applied the changes we discussed.

Elizaveta239 · 2017-09-06T12:33:41Z

@fitermay Thank you! I'll review it this week.

Elizaveta239 · 2017-09-11T12:38:24Z

@fitermay It's ok, I've passed a build to our QA team.

fitermay · 2017-09-11T14:48:13Z

@Elizaveta239 Great, I think getting feedback from QA and EAP users ASAP is the best way to get this right.

Elizaveta239 · 2017-10-04T11:14:23Z

@fitermay Thank you for your contribution! Pull request is merged.

…as the context fixed #593 GitOrigin-RevId: 5ceeb430a4ce9251aa4cd399a017c024255dc6be

fitermay force-pushed the console-history branch 2 times, most recently from d5fa5bc to 38afd5c Compare June 19, 2017 06:44

trofimander requested a review from gregsh June 27, 2017 08:51

trofimander requested review from trofimander and Elizaveta239 and removed request for gregsh August 2, 2017 15:14

fitermay force-pushed the console-history branch from 38afd5c to 971673d Compare August 23, 2017 00:26

ilya-g reviewed Aug 30, 2017

View reviewed changes

Elizaveta239 reviewed Aug 30, 2017

View reviewed changes

fitermay added 8 commits September 1, 2017 19:23

PY-10985/IDEA-104596 extract ConsoleHistoryModel interface

14541ab

PY-10985/IDEA-104596 provide extension point for console history model

92ac71b

PY-10985/IDEA-104596 initial impl of prefix based history model

1618296

PY-10985/IDEA-104596: Pick up master changes after rebase

d3c0d7c

PY-10985/IDEA-104596: migrate to kotlix lib

60305ca

PY-10985/IDEA-104596: Get rid of pcollections dependency

5ff9eef

PY-10985/IDEA-104596: Cleanup kotlin code

f141d68

work around KT-19830

1119167

PY-10985/IDEA-104596: Changes after review

487783c

Replace kotlinx immutable with regular collections Change navigation to previous to be previous seen entries regardless of prefix

fitermay force-pushed the console-history branch from 971673d to 487783c Compare September 6, 2017 00:52

vlasovskikh assigned Elizaveta239 Sep 7, 2017

Elizaveta239 closed this Oct 4, 2017

SergeyZh pushed a commit that referenced this pull request Oct 9, 2020

changed all generated method bodies to have the corresponding method …

4760243

…as the context fixed #593 GitOrigin-RevId: 5ceeb430a4ce9251aa4cd399a017c024255dc6be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PY-10985/IDEA-104596: Prefix based console history #593

PY-10985/IDEA-104596: Prefix based console history #593

fitermay commented Jun 19, 2017 •

edited

fitermay commented Jun 20, 2017

Elizaveta239 commented Aug 11, 2017

fitermay commented Aug 12, 2017 •

edited

Elizaveta239 commented Aug 15, 2017

fitermay commented Aug 15, 2017

fitermay commented Aug 23, 2017

ilya-g commented Aug 30, 2017 •

edited

ilya-g Aug 30, 2017 •

edited

fitermay commented Aug 30, 2017 •

edited

fitermay commented Aug 30, 2017 •

edited

Elizaveta239 commented Aug 30, 2017

Elizaveta239 Aug 30, 2017

fitermay commented Aug 30, 2017 •

edited

Elizaveta239 commented Sep 5, 2017

fitermay commented Sep 6, 2017

Elizaveta239 commented Sep 6, 2017

Elizaveta239 commented Sep 11, 2017

fitermay commented Sep 11, 2017

Elizaveta239 commented Oct 4, 2017



		override fun addToHistory(statement: String?) {
		val stmt = statement ?: return

PY-10985/IDEA-104596: Prefix based console history #593

PY-10985/IDEA-104596: Prefix based console history #593

Conversation

fitermay commented Jun 19, 2017 • edited

fitermay commented Jun 20, 2017

Elizaveta239 commented Aug 11, 2017

fitermay commented Aug 12, 2017 • edited

Elizaveta239 commented Aug 15, 2017

fitermay commented Aug 15, 2017

fitermay commented Aug 23, 2017

ilya-g commented Aug 30, 2017 • edited

ilya-g Aug 30, 2017 • edited

Choose a reason for hiding this comment

fitermay commented Aug 30, 2017 • edited

fitermay commented Aug 30, 2017 • edited

Elizaveta239 commented Aug 30, 2017

Elizaveta239 Aug 30, 2017

Choose a reason for hiding this comment

fitermay commented Aug 30, 2017 • edited

Elizaveta239 commented Sep 5, 2017

fitermay commented Sep 6, 2017

Elizaveta239 commented Sep 6, 2017

Elizaveta239 commented Sep 11, 2017

fitermay commented Sep 11, 2017

Elizaveta239 commented Oct 4, 2017

fitermay commented Jun 19, 2017 •

edited

fitermay commented Aug 12, 2017 •

edited

ilya-g commented Aug 30, 2017 •

edited

ilya-g Aug 30, 2017 •

edited

fitermay commented Aug 30, 2017 •

edited

fitermay commented Aug 30, 2017 •

edited

fitermay commented Aug 30, 2017 •

edited