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

Document:getText fixes #4366

Open
wants to merge 4 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@jamshark70
Copy link
Contributor

commented Mar 20, 2019

Purpose and Motivation

  1. Document:getText had a typo -- omitted this. -- so it could only throw an error.
    a. Also, remove action from its argument list.

  2. Restore an asynchronous version of getText that had been commented out, under a new name getTextAsync.

Some history.

The asynchronous version was introduced in 977bd3a on July 18, 2013 (tagged Version-3.7.0-alpha0).

The synchronous version replacing it (578ab14) dates to August 8, 2013, also tagged Version-3.7.0-alpha0.

So the asynchronous version existed for only about three weeks and was superseded before its alpha release. It's a safe bet, then, that no user code relies on the asynchronous version.

And, the synchronous version wasn't usable (edit). So it's a safe bet that no user code is relying on the current, synchronous version.

So I think my approach here is reasonable -- clean up getText (and remove the async response function action argument, which in fact is currently ignored).

Which leaves the question, why do we need getTextAsync? Because there is currently an undiagnosed and unfixed bug in document text mirroring in Windows. (I've raised this on sc-dev, but it's not getting much attention.) If you need the complete document contents, the only way to do this reliably in Windows at present is to ask the IDE process asynchronously for the contents.

I would suggest, until we are 100% sure that document text mirroring is reliable on all platforms, that we should keep the asynchronous approach around as a backup. User impact in my case is that I was accessing document contents in order to save them as part of a JIT patch (see https://github.com/jamshark70/JITModular/tree/topic/patch). In Windows, aDocument.string is frequently incomplete, meaning that students' code could be lost forever... not quite usable in the classroom.

Types of changes

  • Documentation
  • Bug fix
  • Breaking change (? maybe? explained above)

To-do list

  • Code is tested
  • All tests are passing (added unit tests)
  • Updated documentation
  • This PR is ready for review
@muellmusik

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Apologies James, this was all done a rather long time ago and my memory is hazy. My rough recollection is that:

  • Document was broken after the intro of the IDE, a state which lasted for some time.
  • As it had been broken, it seemed an opportunity to rework things (it was a bit of mess IIRC)
  • The initial idea was to make it all async
  • In the process of doing that it became clear that a synchronous version was possible.
  • There were a number of questions remaining about interface and approach that were never resolved in the discussions that ensued.

getText is semi-duplicated by text, which didn't have the typo, but lacked the range args. I think that was one of the questions to be decided. The actions may have been from the intial async approach.

Going forward, I would suggest adding the start and range args to text with defaults so that existing uses are unaffected. As getText has been broken since the reintroduction of Document, and undocumented, I would just remove that and adjust your unit tests to use text.

As to the async approach, I'm not sure what to suggest. There are definitely some issues, and it may be that it would have been better to go with the async in any case, but there's a fair amount of code that depends on sync. Having an async option doesn't hurt I suppose.

@jamshark70

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Thanks for the background. It looks like the interface needs a general review. I see:

  • string and selectedString (with setters)
  • text, text_, insertText
  • getChar, setChar (? -- I'd argue to remove these. getChar comes from text streaming interfaces, File and CollStream -- Document is orthogonal to that.)
  • getText, setText

That's a lot of duplication. I totally missed text while scanning the method list in Document.browse.

Also consider compatibility with TextView, which has string, string_, selectedString (+setter), setString. (Btw range-selection methods are not compatible between Document and TextView -- https://github.com/jamshark70/ddwSnippets/blob/master/ddwSnippets.sc#L84-L90).

From that perspective, it might be best to dump (deprecate) all of the text methods and make Document's string methods handle all the requirements (and add setString). (Perhaps the Cocoa Document class used text methods, but as noted, that was broken for a long time and then only partially implemented -- highly unlikely that anybody is using them successfully.)

Async: Synchronous string-getters are, of course, better -- provided that they work. In 2013, it seemed to be working on Mac and Linux (I haven't seen any problems with it in Linux), and not extensively tested in Windows, so there was no apparent reason not to go forward with that. Just recently I find that I can break it often in Windows (though not on demand). I have a feeling it's going to be difficult to find the cause -- so we should have a way to query the real owner of the text.

Ironically, it's lucky for me that the feature was not finished. If it had been, somebody might have said "OK, now we can remove the text-access handlers from the IDE backend" (before the feature was thoroughly tested) and then I would have no alternate approach today.

@muellmusik

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Consistency with TextView seems worthwhile.

At the time this was done, there were related questions about Document in other editors. Not sure where that's at these days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.