Foundation for improved textdungeon #14
Conversation
|
tested, but didn't review the code as it's hard to review a huge change in one commit. Please make shorter commits with right commit practices. Please add description to the PR.
Also, please rename the buttons appropriately. |
- previously it was repeating the room description after each command - and minor changes
- Add maps for easy-medium-hard - Change handling of json file
|
Hey, I built a new game-engine from the ground up, so I don't really know how I could make smaller commits. My thought process was to make a commit every time I made a significant addition to the code. |
|
You've removed the copyright and license in activity.py. That alerted me to a copyright compliance issue, but I also see that you have removed almost everything and replaced it with an entirely new implementation. Please add your copyright and license, and if absolutely everything was removed then remove the previous copyright authors. The COPYING, README.md, activity/activity.info files can be ignored. setup.py can keep Simon's copyright and license, though it is trivial. You've added some graphics. Previously this was a text game only, and the activity name has text in it. @chimosky, should this be a new activity rather than a replacement? Perhaps we need a feature table, with columns named feature, before-pull-request, after-pull-request. If no features are lost, and features are added, then I'm fine with proceeding. I don't think smaller commits would be helpful in this situation. Reviewers will just have to look at the new code rather than the differences. None of the open issues are mentioned. Can you please recheck to see if those issues are solved? |
activity.py
Outdated
|
|
||
| # Create three buttons | ||
| for i in range(1, 4): | ||
| button = Gtk.Button(label="Button " + str(i)) |
There was a problem hiding this comment.
Here you create a button, but you then delete it in the lines that follow.
| orientation=Gtk.Orientation.VERTICAL, spacing=6) | ||
| overlay.add_overlay(self.vbox) | ||
| self.vbox.set_halign(Gtk.Align.CENTER) | ||
| self.vbox.set_valign(Gtk.Align.CENTER) |
game_engine.py
Outdated
| self.current_room = Room( | ||
| self.data["rooms"].get(self.data["currentRoom"]), self.data, self) | ||
|
|
||
| self.print(self.data['openning']) |
| self.game_window = game_window | ||
|
|
||
| if not self.data: | ||
| sys.exit('Error loading map file.') |
There was a problem hiding this comment.
sys.exit is not standard for Sugar activities, but as it indicates a software configuration issue that shouldn't be a problem.
There was a problem hiding this comment.
My role is review, not gatekeeper. I see something unusual, I'll call it out. Calling sys.exit is not standard; the outcome may not be as predictable as the more normal termination methods, because exit handlers will be called in an order defined by their setup in the Sugar toolkit. We don't generally test the use of sys.exit, so if a fault is present or does develop in the Sugar toolkit your activity may be affected. The usual way to exit is to call an Activity class method, which triggers the GTK main loop to exit, and whatever called that loop regains control and calls sys.exit eventually.
game_engine.py
Outdated
| except json.JSONDecodeError: | ||
| sys.exit('Error decoding the map file.') | ||
|
|
||
| """This function writes changes in game data (game state) |
There was a problem hiding this comment.
Docstrings should be short; use imperative mood, and don't describe the language structure that follows; i.e. remove "This function", and use "Write" instead of "writes". Also, "game state" is more correct and shorter than "game data (game state)".
Same applies to other docstrings, not just this one.
| sys.exit(f'Issue with copy_{filename}') | ||
| else: | ||
| try: | ||
| shutil.copyfile(filename, self.copy_filename) |
There was a problem hiding this comment.
Sugar activities must be careful to write only to the activity root. When installed in /usr/share/sugar/activities the activity won't be able to write into maps/.
Why is a file being copied?
There was a problem hiding this comment.
I overlooked that and will address that. The .json file is being copied and a .tmp file is created to be used for data storing. Said file, is updated to reflect the game's state
|
Should you derive the maps from the previous activity, please include original author copyright inside them. Quick review of code done. Please fix flake8 issues. Also, if you obtained the engine from somewhere else, please license and credit it. The engine does seem to have been written for an execution context other than Sugar. |
|
|
"include original author copyright inside them" means to comply with the recommendations of the license to ensure that the source code (or data in this case) is properly marked with author and copyright. Please read the license, all the way through, especially the section How to Apply These Terms to Your New Programs. You must read the license if you plan to contribute anything to a piece of software. Experienced developers check the license against known licenses, to make sure they are intact, and then proceed thinking to themselves; "okay, this is GPLv2+, familiar". As the map is in JSON format, you might want to add a comment keyword, and use it first. For other options, search for how other people have added comments to JSON. A search for adding copyright irritatingly discovers several situations where a copyright symbol is being added. The license shows that you don't need to use a symbol, you can just use 'Copyright (C)' which means the same. |
|
@quozl said;
I haven't really tested this to know if some features have been lost but I'm thinking that's the case as this is no more a text based activity, making it a new activity sounds like a good idea. |
|
It is still text based, just not as bare of imagery as before. I suggest you review the repository before and after the pull request commit(s) are applied. It is a very small activity, shouldn't take longer than having to discuss it further. 😁 |
Looked at the activity and I think we shouldn't create a new activity from the changes as it's still text based. I haven't tested these changes yet and I'll have more to say about feature addition/removal but I doubt they've been removal of existing features as it's still text based and the only major change at the moment is the introduction of images. |
COPYING
Outdated
There was a problem hiding this comment.
If you're making updates to the license then let the indentation be as readable as the previous one.
I'm not sure about changing the license of the activity, @quozl thoughts?
There was a problem hiding this comment.
I used the GNU GENERAL PUBLIC LICENSE from https://www.gnu.org/licenses/gpl-3.0.en.html and maintained it's format
There was a problem hiding this comment.
An easy mistake to make if you are unfamiliar with the correct practice.
| # Copyright (C) 2023 Dimitios Mylonas | ||
|
|
||
| # Copyright (C) 2009, Simon Schampijer | ||
| # |
There was a problem hiding this comment.
You removed the original author when his code is still part of this file and you haven't removed everything he wrote.
There was a problem hiding this comment.
I wanted to maintain the same author throughout the files. And since I changed almost everything and the setup file is the default used for every activity, I didn't see a reason for keeping the original author's copyright for it.
There was a problem hiding this comment.
The best reason for keeping the original author's copyright on an otherwise unchanged file is to avoid fraudulent dealing with copyright, which is an offence socially in some online cultures, and legally in some countries. Please make sure you understand copyright law and have fully read and understood the license you are using.
|
|
||
| class TextdungeonActivity(activity.Activity): | ||
| def __init__(self, handle): | ||
| activity.Activity.__init__(self, handle) | ||
| super().__init__(handle) |
There was a problem hiding this comment.
A sugar activity must implement a new class derived from sugar3.activity.activity.Activity, why'd you change this?
There was a problem hiding this comment.
The inheritance hasn't changed, only the method of calling the parent's init has. Did I misunderstand something?
|
Reviewed, c494794. Not tested. I'm yet to review the new engine and I will. Please stop creating PRs with the master branch as this makes it a bit difficult to do some things, see our contributing guide. |
|
Only thing left from the original seems to be a comment in map2.json that it was built from an original map copyright Simon Schampijer, but as far as I recall the activity was written by Tony Forster. You would need his written consent to relicense as GPLv3. It would be better to remove map2.json and treat this contribution as a new activity rather than maintenance of an existing activity. The work by Tony Forster, Tony Anderson, Alexandru Mihai, Rudra Sahu, Rishikesh Joshi, Ayush Nawal and myself has been deleted. The COPYING does not look like the standard GPLv3 file; there are whitespace changes. Remove them and use the standard file. You can find one in the Sugar repository. If the old activity is to be removed, https://help.sugarlabs.org/text_dungeon.html will need to be updated. By the way, there is also the Castle activity. http://activities.sugarlabs.org/addon/4397 |
Also - Change their location to be in activity's root - Remove test map - Add license of original author to map2 file - Make small changes to map3 file
- Add the GNU GPL header to the file - Add new global constants - Split large methods into smaller ones - Introduce CSS for button styling - Refactore the methods for creating the menu and game screen
- Add copyright - Use imperative mood for comments - Change handling of json to correct issues with outputs
Move create_text_view, create_entry_field, create_toolbar to be right after the function they where called in.
Yep, you mentioned that in another PR as well, I misundersood the contributor's guide but now I'm aware. This PR was made when I was unaware.
Ok no problem I can replace it with a different map
chimosky also mentioned that, but since I got the COPYING from https://www.gnu.org/licenses/gpl-3.0.en.html, I thought it would be fine. Moving forwards
|
We use plain text format for COPYING file, as does everyone else who uses GPL. On the page you reference you will find a "plain text" link. Automated license checkers often rely on the plain text form. |
Yeah, I looked properly and noticed the drastic changes and I agree with making this a new activity instead. We could use this PR to test and finalize so everyone can be involved but it'll never be merged, instead @Dimi20cen can create a repo which he can transfer to Sugar Labs after we're done with the reviews here. |
|
How should I proceed then? |
|
As said, create a repository, insert your code into it, let us know the repository path, and instruct GitHub to transfer to Sugar Labs. |
|
The path to the repo is, https://github.com/Dimi20cen/textquest, and now I wait for persmission to create public repositories? |
|
Great. Next, transfer the repository to Sugar Labs. Instructions are at https://docs.github.com/en/repositories/creating-and-managing-repositories/transferring-a-repository |
|
I have reviewed the repository to the commit Dimi20cen/textquest@9668294 in order to verify compliance with Sugar Labs licensing requirements. The only unusual thing I noted was missing end of line in activity.info file, hightlighted by GitHub in the commit view as a red circle with minus sign. |
|
Oh yeah, fixed it |
|
Hey, do I need to do something specific to get permission to create public repositories on sugarlabs? |
|
https://docs.github.com/en/repositories/creating-and-managing-repositories/transferring-a-repository says in part;
Have you arranged for that permission yet? |
|
No, that's what I'm asking. I thought you would grand it to me. |
|
Thanks for letting me know. Please transfer the repository to me, and I'll transfer it to Sugar Labs. |
|
Did it, thank you. |
|
https://github.com/sugarlabs/textdungeon/issues still unfixed, if you'd like to contribute any fixes to textdungeon to fix them, that would be great! |
This PR contains a completely different game-engine.
The new engine is meant to be much more modular. It's also meant to work with .json instead of .txt files. Other changes are the addition of a menu interface and background for each map. Currently there is only one map avalaible, "map1.json". Tomorrow 2 more will be added for the medium and hard difficulty.
@chimosky @sourabhaa