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

Add text tool #64

Merged
merged 17 commits into from
Jun 2, 2021
Merged

Add text tool #64

merged 17 commits into from
Jun 2, 2021

Conversation

ClFeSc
Copy link
Contributor

@ClFeSc ClFeSc commented May 30, 2021

Closes #35

@ClFeSc
Copy link
Contributor Author

ClFeSc commented May 30, 2021

The test testSimpleText is known to fail. This will be fixed.

@ClFeSc ClFeSc marked this pull request as draft May 30, 2021 18:02
@kpostnov
Copy link
Contributor

Without having looked at the code yet, UiFugueIcons >> editIcon seems like a more fitting icon for the text tool.

@kpostnov
Copy link
Contributor

I don't know if it's just me but, in comparison to other branches, something is affecting the performance while drawing. Might be something to take a closer look at.

@ClFeSc ClFeSc force-pushed the feature/35-text-tool branch from 4efe986 to 6b7900e Compare June 1, 2021 15:28
@coveralls
Copy link

coveralls commented Jun 1, 2021

Pull Request Test Coverage Report for Build 900165383

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 194 of 199 (97.49%) changed or added relevant lines in 45 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.6%) to 82.186%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/SketchMorph2-Core.package/M2Backend.class/instance/toggleFillGeometricForm.st 0 1 0.0%
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/chooseColor.st 0 1 0.0%
packages/SketchMorph2-Core.package/M2Backend.class/instance/chooseFont.st 0 3 0.0%
Totals Coverage Status
Change from base Build 895514137: 2.6%
Covered Lines: 1015
Relevant Lines: 1235

💛 - Coveralls

@ClFeSc ClFeSc marked this pull request as ready for review June 1, 2021 16:29
@ClFeSc
Copy link
Contributor Author

ClFeSc commented Jun 1, 2021

We currently have the problem that, when moving MorphicMonet, one can see the temporaryTextMorph floating around in the Image, approx. -100 asPoint away from the top left corner of the Canvas.

@DevSchmidtchen
Copy link
Contributor

We currently have the problem that, when moving MorphicMonet, one can see the temporaryTextMorph floating around in the Image, approx. -100 asPoint away from the top left corner of the Canvas.

This temporary placement at -100 asPoint looks a bit tacky to me. Isn't there another way of hiding the morph at the beginning and between (for example deleting it and only creating it if used)?

@ClFeSc ClFeSc requested a review from DevSchmidtchen June 2, 2021 12:44
@kpostnov
Copy link
Contributor

kpostnov commented Jun 2, 2021

When clicking the Choose Font - button while having the text tool activated it is possible to draw on the canvas even though only writing text should be allowed.

@ClFeSc
Copy link
Contributor Author

ClFeSc commented Jun 2, 2021

When clicking the Choose Font - button while having the text tool activated it is possible to draw on the canvas even though only writing text should be allowed.

Should be fixed.

interaction
dismissCurrent
self canvas ifNotNil: [self canvas removeMorph: self].
ActiveHand newKeyboardFocus: nil.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is being set twice. Once in the sender of dismissCurrent and once here. I suggest removing this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest removing it in the sender since we are also setting the focus in M2TemporaryTextMorph>>getAt:, so I think the same class should be responsible for releasing the focus.

@ClFeSc ClFeSc requested a review from kpostnov June 2, 2021 18:25
Copy link
Contributor

@maximilian-franz maximilian-franz left a comment

Choose a reason for hiding this comment

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

I don't quite get the need for M2TemporaryTextMorph, what is the benefit of having such a temporary text morph, moving it to the target position and eventually creating a new Textmorph to print the text over simply creating a TextMorph at the target position and printing its contents?

@@ -0,0 +1,6 @@
interaction
getAt: aPoint
Copy link
Contributor

Choose a reason for hiding this comment

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

getAt: sounds weird to me since the morph already exists. Maybe use moveTo: instead?

getAt: aPoint
self position: aPoint.
self canvas ifNotNil: [self canvas addMorph: self].
ActiveHand newKeyboardFocus: self.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ActiveHand newKeyboardFocus: self.
self activeHand newKeyboardFocus: self.

@maximilian-franz maximilian-franz self-requested a review June 2, 2021 19:35
@ClFeSc ClFeSc force-pushed the feature/35-text-tool branch from b7fad04 to cbf1da9 Compare June 2, 2021 19:47
@ClFeSc ClFeSc merged commit 278ec04 into dev Jun 2, 2021
@ClFeSc ClFeSc deleted the feature/35-text-tool branch June 2, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text-Tool (21)
7 participants