Community
Participate
Working Groups
Testing frameworks like SWTBot or Red Deer need to locate widgets and perform some actions on them. In case of text widgets like TextCanvas, the key actions are - type to it - read its content TextCanvas doesn't provide a method that would allow the latter. For now, I'm using a workaround via clipboard: textCanvas.selectAll(); content = textCanvas.getSelectionText(); but it's quite awkward. Please provide a public method to read the content.
I started a related thread on the dev mail list: https://dev.eclipse.org/mhonarc/lists//tm-dev/msg01219.html
Created attachment 268695 [details] minor refactoring in AbstractTextCanvasModel This is just a minor refactoring patch. An actual change will follow.
Created attachment 268696 [details] actual change: TextCanvas.getTextContent() Add TextCanvas.getTextContent(). Depends on the previous patch.
The two patches above seem to solve the RFE for me, applied on master (36dc12036a6c56801e7ee0adc70303a414bba14b). The code builds and all test suites I found pass: - Terminal All Unit Tests.launch - Terminal AutomatedTests.launch - Terminal Plugin Tests.launch However, it seems to me that the classes I patched aren't covered by the test suites, at least not directly. So I haven't added any test case...
Hello Vaclav, Could you please make your contribution through Gerrit? - That makes it easier to discuss the changes. Instructions here: http://wiki.eclipse.org/Gerrit https://git.eclipse.org/r/p/tm/org.eclipse.tm.terminal I've just had a look at your first patch - the minor refactoring - and I'm not entirely happy with it because you removed this from the original code: // get rid of the empty space at the end of the lines // text=text.replaceAll("\000+$",""); //$NON-NLS-1$//$NON-NLS-2$ // <J2ME-CDC-1.1 version> This comment meant to say that the complex iterating over characters was just done because the String#replaceAll() method isn't available in J2ME/CDC-1.1 Losing this comment, it's not clear any more that at some point the code could perhaps be simplified again. I also don't quite understand why you added the null check in your scrubLine() ? The original code had no null check, so I'd assume that text cannot be null in this situation? Finally, it's getting very late for Oxygen, we need to contribute RC3 on Monday. Could you live with your change not making the Oxygen release? Thanks, Martin
Regarding your 2nd contribution: /** * @return all text, the lines separated by '\n' Please add a real Javadoc here -- 1st line should be the brief method description, optionally followed by a longer description. In your case, I'd expect a statement that this method is designed for test automation. Your getAllText() method doesn't look very efficient, converting between char[] / String / StringBuffer. That's OK when it's primarily for testing. But other parts of the Terminal are tuned for efficiency. In this case, is a String really the best possible return value of your API? Or should it be an ArrayList<String> for example with a separate String per Line, to enable more String sharing? Or maybe a String[] ? Also, why do you use 2 different method names, getAllText() vs. getTextContent() ? Please add Javadoc for the 2nd one as well. Adding a few comments what you do here and why would be beneficial IMO. Then, resubmit via Gerrit. Thanks!
New Gerrit change created: https://git.eclipse.org/r/98500
Thank you Martin for your review! > Could you please make your contribution through Gerrit? Posted. (Also took the liberty to join the two patches into just one.) > I've just had a look at your first patch - the minor refactoring - and > I'm not entirely happy with it because you removed this from the original > code: ... You are absolutely right, that was a gross misunderstanding on my side. Now should be 1:1. > I also don't quite understand why you added the null check in your > scrubLine()? The original code had no null check, so I'd assume that > text cannot be null in this situation? More of a my (bad) habit than anything else, when I extract code to a new method I often add something like that to anticipate calls from new situations. I see it can impact performance. The new caller I'm adding does not need the check either, so I removed the check. > Finally, it's getting very late for Oxygen, we need to contribute RC3 on > Monday. Could you live with your change not making the Oxygen release? Not a problem. I have been living with my current workaround for quite some time :-) > Regarding your 2nd contribution: > > /** > * @return all text, the lines separated by '\n' > > Please add a real Javadoc here -- 1st line should be the brief method > description, optionally followed by a longer description. In your case, I'd > expect a statement that this method is designed for test automation. This was intentional, most of the neighbouring code has similarly shot Javadocs and I didn't want to add any visual prominence to the new code as it's not the "real" API. Anyway, I rewrote my Javadocs. > Your getAllText() method doesn't look very efficient, converting between > char[] / String / StringBuffer. That's OK when it's primarily for testing. > But other parts of the Terminal are tuned for efficiency. In this case, is > a String really the best possible return value of your API? Or should it be > an ArrayList<String> for example with a separate String per Line, to enable > more String sharing? Or maybe a String[] ? This is something I actually had thought about. Just from a "taste" POV, I'd probably slightly prefer ArrayList<String>. However, String works for me equally well and, in the end, other reasons prevailed: - align the API with getSelectedText() - returns String with NLs, too - as a consequence, I could share the implementation logic with getSelectedText() - to return List<String>, I'd probably need some StringBuffer as an intermediate anyway - to cope with wrapped lines But I'm open to change it if you really think it's worth it. > Also, why do you use 2 different method names, getAllText() vs. > getTextContent()? No reason. getTextContent() was historically first, getAllText() came later as a good fit in ITextCanvasModel. I just didn't came to me to unify them. Fixed now. > Please add Javadoc for the 2nd one as well. Adding a few comments what > you do here and why would be beneficial IMO. I tried. > Then, resubmit via Gerrit. Thanks! Done. Once again, thanks for you very helpful review! Vaclav
Hi Martin and Vaclav, any news about this issue? Thanks Josef
(In reply to Josef Kopriva from comment #9) > Hi Martin and Vaclav, > > any news about this issue? > > Thanks > Josef The patch proposed in Gerrit is still valid. I'm trying to reach Martin in a PM. If I don't hear from him I'll bring this to the tm-dev mail list... Vaclav
... > The patch proposed in Gerrit is still valid. I'm trying > to reach Martin in a PM. If I don't hear from him I'll > bring this to the tm-dev mail list... Brought to the ML: https://dev.eclipse.org/mhonarc/lists/tm-dev/msg01246.html Vaclav
Gerrit change https://git.eclipse.org/r/98500 was merged to [master]. Commit: http://git.eclipse.org/c/tm/org.eclipse.tm.terminal.git/commit/?id=3ee6618da1282970ebb386f622c9c5b89ae0ab0b
I have merged the contribution; sorry for the long delay. Only the plug-in and features versions, as well as the new @since tags, need to be updated to 4.4 since you are adding new API. I'll do that with the next commits.
(In reply to Martin Oberhuber from comment #13) > I have merged the contribution; sorry for the long delay. Thank you!
The releng is updated, a first 4.4milestones build is available here for "update": http://download.eclipse.org/tm/terminal/updates/4.4milestones I think we can consider this done. Many thanks for the contribution!
*** Bug 283298 has been marked as a duplicate of this bug. ***
