4712-Text-Core-package-documentation-and-test

[hilaire]

WIP #4712
On TextStream

[Ducasse]

This is cool!

[MarcusDenker]

should we merge this already?

[jecisc]

Cool!

I suggested some changes to have better logs in the CI in case it fails in the future.

[hilaire]

@MarcusDenker About merging, I don't know but I have not finished. Is it ok after merge to add commit to this PR or should I open another PR?

[jecisc]

You can still add commits on the branch, but you’ll need to open a new PR

[hilaire]

@jecisc, I fetched these 3 tiny changes and Iceberg is pulling a full set of changes not related. I specifically asked a pull for these 3 changes. Took too long and I have now unwished changes in my image I will have to pay attention on when commiting.

Btw, I can't create a new PR on the same branch. It is refused. Or do I miss something?

[hilaire]

@jecisc , looks like the commit got it way to the existing PR even if merged. But was it merged after all?

[hilaire]

See the comment of TextAttribute. What is still relevant in this comment

Tells a piece of text to be a certain way.

Select text, press Command-6, choose a attribute.  If selected text is of the form 
	Hi There<Smalltalk beep>
the part in angle brackets is saved for action, and the Hi There appears in the paragraph.  If selection has no angle brackets, use the whole thing as both the text and the action.

TextDoIt  --  eval as a Smalltalk expression (the part in angle brackets)

TextLink -- Show a method, class comment, class hierarchy, or class defintion.
	<Point extent:>, <Point Comment>, <Point Hierarchy>, or <Point Defintion> are what you type.

TextURL -- Show the web page. <www.disney.com>

These attributes of text need to be stored on the disk in a regular file-out.  It is done in this form: 	Hi There   
	in the text, and a Run containing   dSmalltalk beep;;
	Click here to see the extent:   
	in the text, and a Run containing   method LPoint extent:;
See RunArray class scanFrom: where decoding is done.

[hilaire]

I propose this new comment for TextAttribute. Once I learn enough of TextAttribute I can amend it.

I am an abstract class to represent a text attribute, the way a portion of text is graphically represented.
My sub-classes specify these representations:
- colouring (TextColor), 
- bold, italic, underlined, 
- narrow, strike out (TextEmphasis), 
- alignment (TextAlignment),
- indentation (TextIndent),
- font change (TextFontChange),
- kerning (TextKern),
- url and related action (TextAction hierarchy).

My instances are stored in the Text instance runs variable (a RunArray).

[hilaire]

I see the PluggableTextAttribute with no user. Do we need it? Can we deprecate it?

[MarcusDenker]

Should this be merged?

I propose to do small PRs that we can merge fast... especially tests and comments

[hilaire]

Yes, you can merge. I will come back to it later then.

[MarcusDenker]

Two tests are failing:

testNoUnusedTemporaryVariablesLeft
the following methods have unused temporary variables and should be cleaned: an OrderedCollection(TextEmphasisTest>>#testDominates)

SystemDependenciesTest>>testExternalMorphicDependencies
Given Collections do not match! additions : #(#'WebBrowser-Core') missing: #()

[hilaire]

Fix the first failing test. The second fail, AFAIK, has nothing to do with my commits in this pull.

[Ducasse]

Hilaire I do not get why when I look at the files changed I have
HashAndEqualsTestCase >> testEquality [
"Check that TextFontChanges report equality correctly"
"Check TextAttribute subclasses with equality message report it correctly"

????

[Ducasse]

also SycAbstractAllInstVarAccessorsCommand class

[hilaire]

Ah this is unwished comits. Can you delete these changes?

[Ducasse]

No we cannot (for now and the soon future) cherry pick on merge.

[hilaire]

Fix it.
Is there any left unwished changes?

[Ducasse]

Thanks Hilaire.
Let's me check.

[Ducasse]

Did you remove this change

@@ -21,7 +21,7 @@ HashAndEqualsTestCase >> setUp [

{ #category : #tests }
HashAndEqualsTestCase >> testEquality [
"Check TextAttribute subclasses with equality message report it correctly"
"Check that TextAttribute subclasses report equality correctly"
prototypes
do: [:p | self
should: [(EqualityTester with: p) result]]

because it looks still there.

[hilaire]

That's correct, I updated the comment.

[Ducasse]

Ok

[MarcusDenker]

nevertheless, now in the build we have two tests failing that did not fail before:

#4818
#4819

I checked and for the SystemDependencies, it is indeed this PR that makes them faill

[jecisc]

Was this class removal wanted?

[hilaire]

I don't understand this change. At some point, Iceberg did some unwished changes to my working image. I tried to revert it. Was it a left over, I can't tell for sure.

[tesonep]

We have to remove the dependency of WebBrowser from Text, it does not have any sense there.
It should be handled by the block in the text element.
If not, we will revert the whole PR

[Ducasse]

Yes this is strange since I did not see the dependencies when I checked the tests.
@cyril probably hilaire wanted to remove a change from within his PR and it was converted to a removal.
I should say that I do not really know how to use iceberg for cherry picking.

[Ducasse]

Pablo technically can hilaire still do it in his PR or should we create a new issue to fix it.

[hilaire]

TextUrl attribute needs to know about WebBrowser to open an URL, so there is sense.
Will adding a "Smalltalk tools webBrowser" solve the dependency?

[hilaire]

@tesonep, what about my previous question?

TextUrl attribute needs to know about WebBrowser to open an URL, so there is sense.
Will adding a "Smalltalk tools webBrowser" solve the dependency?

If so I can open an issue.