Skip to content

Conversation

johannesnydahl
Copy link
Contributor

To generalize kvadrat and stapel so that the pen and fill color can be specified, one needs to know the type of color variables in Kojo. For students to figure the type out, they either have to study the Kojo source code or use Metals' type derivation.

I think the type should be specified in this task to minimize confusion.

@bjornregnell
Copy link
Member

bjornregnell commented Sep 25, 2024

Good point! Is it java.awt.Color or some other Kojo-package with own Color? (I've forgot about this detail as it was a while ago I touched the Kojo internals...) I guess it would be good to state the full package path of the type if you want to take a look in the Kojo code and update your PR?

@bjornregnell
Copy link
Member

bjornregnell commented Sep 25, 2024

I've just checked. It is java.awt.Color. Probably we should import it in the file where other stuff from kojo is imported...
EDIT: export not import

@johannesnydahl
Copy link
Contributor Author

johannesnydahl commented Sep 25, 2024

Yes. Should I still change PR even if we add export java.awt.Color to workspace file?

@bjornregnell
Copy link
Member

bjornregnell commented Sep 25, 2024

Hmm. We have during the first week not mentioned import but the actual type is java.awt.Color so I suggest to update the PR with the last sentence being something like

Färgerna i Kojo är av typen \code{java.awt.Color}. 
Typen är tillgänglig under namnet Color då namnet är importerat med 
\code{import java.awt.Color}.

to avoid potential confusion.

@johannesnydahl
Copy link
Contributor Author

PR is updated.

@bjornregnell
Copy link
Member

I suggest that you change importerat med export till gjorts direkt tillgängligt med export

@bjornregnell
Copy link
Member

Och jag tror det fattas ett "i" före "vecka 4"

@johannesnydahl
Copy link
Contributor Author

My bad. Should probably be fixed now?

@bjornregnell bjornregnell merged commit 0b59348 into lunduniversity:master Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants