Welcome, Guest. Please login or register.

Pages: [1] 2 3
Print
Author Topic: Code review/complaints/discussion  (Read 19811 times)
b0rland
Community member

Posts: 105



View Profile Email
« on: August 05, 2009, 09:48:46 AM »

Following Kaydeth's proposal, I'm creating this topic as a place to discuss things to be improved in the code. Those that are generally considered a good idea will afterwards become refactoring tickets in Trac.
Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #1 on: August 05, 2009, 10:18:45 AM »

First complaint Smiley.
Dialog window ownership in Hud and World.
In most cases Hud object creates and owns dialog windows (hud.py:38-41), although World accesses them as if they were its own (world.py:72-83). In some cases (e.g. Inventory), World itself owns the dialog window (world.py:87-90).
I fail to see the difference in these cases that requires different approach. My suggestion is for hud to own all the dialogs, for world to only interact with them via hud's functions.
Examples of offending usage:
Code:
        self.hud = hud.Hud(self.engine, TDS)
        self.hud.events_to_map["inventoryButton"] = cbwa(self.displayInventory, True)
        self.hud.events_to_map["saveButton"] = self.saveGame
        self.hud.events_to_map["loadButton"] = self.loadGame
        hud_ready_buttons = ["hudReady1", "hudReady2", "hudReady3", "hudReady4"]
        for item in hud_ready_buttons:
            self.hud.events_to_map[item] = cbwa(self.hud.readyAction, item)
        self.hud.hud.mapEvents(self.hud.events_to_map)
        .....
        inv_data = {'A1':'dagger01', 'A3':'dagger01'}
        self.inventory = inventory.Inventory(self.engine, inv_data, self.refreshReadyImages)

Suggested refactoring:
Code:
       self.hud = hud.Hud(self.engine, TDS)
       # Hud will init Inventory too; inv_data should be fetched from some inventory model object, but that's separate story, can stay hardcoded for now
       events_to_map["inventoryButton"] = cbwa(self.displayInventory, True)
       events_to_map["saveButton"] = self.saveGame
       ....
       self.hud.mapHudEvents(events_to_map)
       ....
       self.hud.mapMenuEvents(events_to_map)
       ....
       self.hud.mapInventoryEvents(events_to_map)
Logged
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #2 on: August 06, 2009, 01:26:17 PM »

Makes sense to me as long as you are only talking about the GUI part of the Inventory. It doesn't make sense to me for the actual contents of the inventory to be owned by the Hud class.

What about context menus and container displays? Should these also be a part of the HUD?
Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #3 on: August 07, 2009, 09:02:19 AM »

Makes sense to me as long as you are only talking about the GUI part of the Inventory. It doesn't make sense to me for the actual contents of the inventory to be owned by the Hud class.
Yes, that was a second proposal I planned to make. Currently both the gui for inventory and the inventory contents storage seem to be done by the same class which is a part of "view". I think Inventory (as in contents storage and management device) will have enough non-trivial logic to move it into separate class inside "model". InventoryGUI should reference and query it when drawing or processing events.

What about context menus and container displays? Should these also be a part of the HUD?
That sounds sane. But I think context menus are implemented on FIFE level and are in a separate hierarchy. That needs some more research.
« Last Edit: August 07, 2009, 09:04:39 AM by b0rland » Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #4 on: August 07, 2009, 09:30:16 AM »

My bad. Context menu is actually in PARPG, so it needs to be owned by HUD too.
Logged
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #5 on: August 07, 2009, 01:47:55 PM »

I think Inventory (as in contents storage and management device) will have enough non-trivial logic to move it into separate class inside "model".

Agreed except I don't think you mean "model" as that is a FIFE class. We'll work that out once the Inventory is actually finished though.
Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #6 on: August 09, 2009, 08:26:53 AM »

Right. I'm referring to the model/view separation of PARPG's code (and I mean model/view as design patterns rather then class names). Supposedly the idea was to make world class the entry point to all view code and engine class to represent the model code.
Logged
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #7 on: August 09, 2009, 07:40:00 PM »

Unless there are any objections or alternate suggestions we'll go ahead and create tasks for these changes.

In summary:

We'll move Inventory, Context Menus, and Container displays into the Hud Class.  Displaying any of these will then require access to the Hud class. In the long run this should be GUI components only. Contents of these components should be created and managed elsewhere.
Logged
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #8 on: August 11, 2009, 10:41:17 PM »

FYI, tickets created:

http://parpg-trac.cvsdude.com/parpg/ticket/78
http://parpg-trac.cvsdude.com/parpg/ticket/79
http://parpg-trac.cvsdude.com/parpg/ticket/80
Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #9 on: August 12, 2009, 06:51:28 AM »

Thanks a lot. I've been reading the code and it seems there's much more than that to untangle. But at least it still seems like a good idea.
Logged
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #10 on: August 12, 2009, 12:59:42 PM »

Thanks a lot. I've been reading the code and it seems there's much more than that to untangle. But at least it still seems like a good idea.

Yep. One step at a time Smiley
Logged
Cruul
Guest
« Reply #11 on: August 14, 2009, 12:49:17 AM »

Looked through the code a bit.

Modified the popup.py code to simplify things and created a patch.
Created a ticket in Trac for the patch #81.

-Cruul
Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #12 on: August 14, 2009, 09:03:04 AM »

Modified the popup.py code to simplify things and created a patch.
Created a ticket in Trac for the patch #81.

Good one. Thanks for the patch. That certainly needed being done.
Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #13 on: August 14, 2009, 09:05:33 AM »

A riddle.

Can anyone guess how are PARPGFileBrowser and FileBrowser classes different? It's not immediately obvious to me.
Logged
Atomic
Community member

Posts: 31



View Profile
« Reply #14 on: August 14, 2009, 09:50:05 AM »

On a similar note gui_container.py and popups.py both have a class ContainerGUI that are about 90% the same.  Anyone know which is the "real" ContainerGUI?
« Last Edit: August 14, 2009, 09:56:17 AM by Atomic » Logged
Pages: [1] 2 3
Print
Jump to: