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

v23 #10

Merged
merged 2 commits into from Jun 29, 2020
Merged

v23 #10

merged 2 commits into from Jun 29, 2020

Conversation

JuiP
Copy link
Member

@JuiP JuiP commented May 12, 2020

Help needed @quozl @chimosky :
I'm confused, activity/activity.info mentions activity_version = 22 but there are no git tags or a NEWS file.
Also I don't see po folder with translation files and the activity is not a project under https://translate.sugarlabs.org/

I was referring to the maintainer's list

@chimosky
Copy link
Member

I'm guessing the activity was moved from it's gitorious instance but I can't find the activity repo and it's also not on sunjammer.
When the repo was created, other measures weren't taken.
Maybe @quozl can help, I can't remember where the OLPC git instance was else I would have checked there too.

@quozl
Copy link

quozl commented May 13, 2020

The author was Peter Hewitt, and he made bundles and did not use git. There was no requirement to use git.

This repository does not follow our conventions on tags or commit history. Look at the commit history and you'll see Tony Anderson built this repository by unpacking the zip file bundles of older versions of the activity.

@JuiP
Copy link
Member Author

JuiP commented May 13, 2020

This repository does not follow our conventions on tags or commit history. Look at the commit history and you'll see Tony Anderson built this repository by unpacking the zip file bundles of older versions of the activity.

So does this mean we don't release this activity like other activities? I haven't released activities before so might have few questions initially.

For releasing the activity,

  • throughly test activity and fix what I can
  • check all points in maintainer's list
  • make a PR with changes in the NEWS file, fetching translations, pot file and changing the activity version number

Is this all/ I have skipped something I should know?

@quozl
Copy link

quozl commented May 13, 2020

Won't really know if you've skipped something until we get to review a pull request. 😁

Sugar Labs hasn't released this activity despite the great work by @pipix51 (2017), @yashagrawal3 (2018), and @pro-panda (2018). And now you've worked on it. Thanks!

Missing a complete commit history (because there wasn't one), or version tags (because nobody made any), doesn't preclude the remainder of the maintainer checklist.

You have attempted to correlate the tags (none) with the activity_version. That won't stop you from creating a tag, or updating the version.

Likewise you can't "apply any translate.sugarlabs.org changes" because there aren't any, in turn because the activity has not been set up for translation. I've looked briefly at the source code, and can't see anything that needs to be translated.

I suggest creating a NEWS file with a summary of the changes made since v22.

You can also add tags for the previous versions; see man git tag for how to do that. Tagging isn't covered by pull requests though.

@JuiP JuiP changed the title Added NEWS file v2 May 23, 2020
@JuiP
Copy link
Member Author

JuiP commented May 23, 2020

@chimosky @quozl kindly review.
I think we should keep updating host_version instead of activity versions for activities like these. The NEWS file can then include all changes made to the activity after it was added as a sugarlabs activity. What do you think?

@quozl
Copy link

quozl commented May 25, 2020

Thanks. But I don't think you understood what host_version was for. It was for declaring the compatible version of OLPC OS, so that bundles won't be installed where they cannot run. The idea was to increment this number in Sugar when a breaking change to the API happened, and then change it in activities that complied with that breaking change.

  • Activity Metadata in Toolkit source is my definitive list of metadata, and no longer lists host_version, it is so old,
  • Content Bundles at Sugar Labs says "The host version is a single positive integer that refers to the version of Sugar which the collection is compatible with. For now, the version is 1. Do not use a different value."
  • Activity Bundles at OLPC says "Each activity.info file must have a "host_version" key. The version is a single positive integer. This specifies the version of the Sugar environment which the activity is compatible with. (fixme: need to specify sugar versions somewhere. Obviously we start with 1.)",
  • Activity Bundles on Sugar Labs does not mention host_version now.

Hope that helps!

@JuiP
Copy link
Member Author

JuiP commented May 31, 2020

Thanks! @quozl
Looking at the links you shared, it seems like host_version no longer required. Do you think I should remove it from the activity.info file?

@chimosky
Copy link
Member

Thanks! @quozl
Looking at the links you shared, it seems like host_version no longer required. Do you think I should remove it from the activity.info file?

Yes you should remove it.

@JuiP JuiP changed the title v2 v23 Jun 2, 2020
@chimosky
Copy link
Member

Reviewed, thanks.

Waiting for @quozl to confirm merge.

@@ -1,7 +1,6 @@
[Activity]
name = Jumble
activity_version = 22
host_version = 1
activity_version = 23
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add a one-liner summary before the next release?

Copy link

Choose a reason for hiding this comment

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

Of course. Do you have one? Push to this branch.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, missed this message. and I don't have a summary :/

Copy link

Choose a reason for hiding this comment

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

http://activities.sugarlabs.org/en-US/sugar/addon/4413 has some text that could be used.

Copy link
Member

Choose a reason for hiding this comment

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

In a jumble of 83 objects, the player has to locate 20. Each deal is different.

I have never played the activity / maybe I don't remember. I will check its appropriateness and create a PR.

Copy link

Choose a reason for hiding this comment

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

I've tested on 1024x768 resolution. I did get situation where object requested was not visible. I don't seem to get any response from right-click, O, or N keys.

Original was for 1200x900 resolution. So the space has changed. Perhaps the number of objects should scale according to the reduced space. Does it? I've not checked.

Perhaps the objects should never be drawn on the edge of the display.

Perhaps the objects should be drawn in the reverse stack order as the challenge? That way the object will always be visible. Seems a bit easy though.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree. That would do good.

Perhaps the objects should never be drawn on the edge of the display.

We could provide a safe display area, subtracting / adding some pixels from Screen.height() and Screen.width()

Perhaps the objects should be drawn in the reverse stack order as the challenge? That way the object will always be visible. Seems a bit easy though.

Agreed. I am not sure if this is what you have suggested, but this is what came to my mind: Sort images on the basis of size of the pixmap in descending order, and then add it. Or did you have any other idea?

Yes, that would solve the issue!! 🎉

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the game was initailly designed for 1024x768 resolution. Guessing from this line.

Perhaps the number of objects should scale according to the reduced space.

The object images are loaded from utils method. They are scaled according to their size. You can see in this line. So this can't be a possible solution.

Another thing I noticed while testing now was: two cursors are seen. Way to reproduce- move the mouse to the toolbar. The mouse cursor on the toolbar is different and larger from the one that is left behind on the game canvas.

Copy link

Choose a reason for hiding this comment

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

Perhaps the number of objects should scale according to the reduced space.

The object images are loaded from utils method. They are scaled according to their size. You can see in this line. So this can't be a possible solution.

You've misunderstood. I don't mean scale the object images, I mean scale the number of objects.

Another thing I noticed while testing now was: two cursors are seen. Way to reproduce- move the mouse to the toolbar. The mouse cursor on the toolbar is different and larger from the one that is left behind on the game canvas.

That's normal for pygame activities. It's another reason not to use pygame.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's normal for pygame activities. It's another reason not to use pygame.

Ohhh, thanks! I treated this as an issue in CowBulls #17 and didn't find a fix, seems like I don't have to treat it as an issue. 😄

@chimosky chimosky merged commit 35d66c9 into sugarlabs:master Jun 29, 2020
@JuiP JuiP mentioned this pull request Jul 6, 2020
6 tasks
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.

None yet

4 participants