Dev Meeting Log 20091017

Summary

• LST Editor design outlined
• Facet work update
• Parsing error reporting improvements outlined

Actions

• None

Transcript

(9:08:54 AM) James[Code_SB]: I'll start on the status then and we can each give a short report
(9:09:55 AM) James[Code_SB]: I have the Spring integration working locally and just need to add it to the build and test before committing
(9:11:05 AM) James[Code_SB]: I've also been playing around with the Jira config to make it work for the code team's needs
(9:11:10 AM) James[Code_SB]: That's about it.
(9:11:12 AM) James[Code_SB]: next
(9:11:56 AM) thpr: I've been focused on the formula bugs (5 of them)
(9:12:16 AM) thpr: to ensure that a formula and PREVAR get evaluated correctly depending on their context
(9:12:34 AM) thpr: it's exposed a few other underlying issues, but I'm mostly there
(9:12:50 AM) thpr: big patch though (nearing 300k), so unlikely to backport any of it to 5.16
(9:13:19 AM) James[Code_SB]: yikes - yes that is major
(9:14:09 AM) thpr: as I already mentioned, I also had a discussion with nuance on the formula system
(9:14:30 AM) thpr: my action there is to take my formula compiler from the sandbox and write a tool that will "test parse" all of the formulas in our data to see if the system can parse them (which is reading them, not interpreting the values)
(9:14:49 AM) thpr: assuming that works, then we can go further down that evaluation. Hopefully any show stoppers will show up at that point
(9:15:02 AM) nuance: I'll be really impressed if it does, I think that's a really hard problem
(9:15:18 AM) thpr: I take that as a challenge :)
(9:15:20 AM) thpr: Next project after that will be CHOOSE rebuild, now that Eddy has finished running the data through the converter
(9:15:40 AM) thpr: that's it from me
(9:15:44 AM) nuance: I haven't done anything project related at all since the last meeting.
(9:15:44 AM) nuance: I owe an update on the features supported by IDEA, though and I hope to
(9:15:45 AM) nuance: update that this weekend.
(9:16:04 AM) thpr: Mark, what IDE do you use?
(9:16:14 AM) nuance: I might give Kar a bit a of a hand with his output clean up while I'm waiting for Tom too
(9:16:49 AM) thpr: We still need to get someone who uses NetBeans to chime in on that features list too
(9:17:05 AM) nuance: If no-one uses it , do we care?
(9:17:07 AM) thpr: I was hoping we could wrestle Connor into that
(9:17:11 AM) MotorViper: I use eclipse
(9:17:20 AM) thpr: Connor uses it
(9:17:23 AM) nuance: Ah, that's right Connor uses it doesn't he
(9:18:24 AM) thpr: I think that may be it for reports James
(9:19:12 AM) James[Code_SB]: Mark, did you want to add anything?
(9:19:32 AM) MotorViper: OK
(9:19:40 AM) thpr: oops, sorry about that :(
(9:19:48 AM) James[Code_SB]: anything you are working on or are you ready for a new assignment?
(9:19:51 AM) James[Code_SB]: :)
(9:20:35 AM) MotorViper: I've been looking at the error reporting prior to starting on the LST editor as it needs to have an easier way of getting feedback into the editor.
(9:21:39 AM) MotorViper: Should also have the side effect of making error handling easier to maintain etc.
(9:22:07 AM) MotorViper: It's going OK but there are a lot of classes to edit!
(9:22:34 AM) thpr: if it hits the tokens, that's a few hundred classes
(9:22:42 AM) MotorViper: Don't I know it!
(9:22:48 AM) thpr: :)
(9:23:01 AM) MotorViper: :(
(9:23:31 AM) James[Code_SB]: Probably best to do an interim commit along the way to get feedback
(9:24:04 AM) James[Code_SB]: rather than do everything and find out that there is tweaking necessary
(9:24:14 AM) MotorViper: Not easy to do an interim commit but I can give you some idea of what I'm doing if you like.
(9:24:30 AM) James[Code_SB]: Sure - we might add that to the end of the agenda?
(9:24:38 AM) MotorViper: OK by me
(9:26:52 AM) James[Code_SB]: ok, next item - the LST Editor
(9:27:46 AM) thpr: Maybe this is one to direct at Mark as well
(9:28:01 AM) thpr: the question being - are you comfortable with what the first steps would be
(9:28:24 AM) thpr: or it might make sense to step through them
(9:28:30 AM) thpr: or at least my idea of them :)
(9:29:33 AM) MotorViper: Lets have your ideas first
(9:29:50 AM) thpr: ok
(9:30:42 AM) thpr: For background, we did discuss the LST Editor in the last meeting: http://tech.groups.yahoo.com/group/pcgen_developers/message/308 for anyone following along at home
(9:31:21 AM) thpr: The general framework is one where the Editor token builds a String and submits that String to one of the CDOMTokens for processing & validation
(9:32:09 AM) thpr: "Advanced" tokens may have some form of UI customized to that token (e.g. a number spinner for an integer value)
(9:32:24 AM) thpr: but any token should be able to be added as a CDOMToken and still appear in the Editor
(9:32:41 AM) thpr: therefore, the Editor needs to know what tokens exist and be able to produce a default for those tokens
(9:32:55 AM) thpr: the default being simply a JTextField that the user can input in the token value
(9:33:23 AM) thpr: so in my opinion, the starting framework is to have a system that uses the existing plugin loader system to initialize the plugins
(9:34:02 AM) thpr: look in gmgen.pluginmgr.PluginLoader for that
(9:34:23 AM) thpr: then provides a list of item types (probably hardcoded for now)
(9:34:43 AM) thpr: item types being Language, Ability, Skill, Spell, etc.
(9:35:17 AM) thpr: When one of those items is edited, there would be 3 tabs
(9:35:32 AM) thpr: first tab is named the type of object (e.g. "Language")
(9:35:41 AM) thpr: it has a field for the item (Language) name
(9:36:10 AM) thpr: second tab is "Other", which has all CDOMTokens that are relevant for a language, but not relevant for a CDOMObject
(9:36:25 AM) thpr: third token is "Global Other" which has all CDOMTokens relevant for CDOMObjects
(9:36:36 AM) thpr: all the tokens are just presented as JTextFields for now
(9:36:53 AM) thpr: That at least has a minimal UI framework for the tokens
(9:37:17 AM) thpr: that framework then needs the ability to edit existing items (vs. creating new ones) and should save the items when it closes
(9:37:35 AM) James[Code_SB]: can each of those tokens be added multiple times? In that case it may be best to have a list of added tokens and the ability to add/edit/remove on the second and third tabs
(9:37:38 AM) thpr: also copy items (& give them a new name)
(9:38:12 AM) thpr: James, good point, some tokens can be used multiple times
(9:38:23 AM) thpr: so it needs to be a list like we have today on the "Advanced" tab
(9:41:05 AM) thpr: I think that describes a minimal framework
(9:41:50 AM) thpr: and doesn't require any of the token UIs to be built in order to demonstrate the features
(9:41:58 AM) MotorViper: I can't see the need for 3 tabs, unless there's a good reason it's easier to use with the "Other" and "Global Other" combined into one
(9:43:02 AM) thpr: that's probably true
(9:43:10 AM) thpr: since the other tabs would be overflow
(9:43:21 AM) thpr: and thus only occur when a UI token doesn't exist, they will be rare
(9:43:24 AM) nuance: If we split those, won't the user have to know which one to add a new entry to?
(9:43:33 AM) thpr: so separation of the global tokens doesn't have that much value
(9:43:52 AM) thpr: The long term goal is to have each of the UI tokens specify what Tab it will appear on
(9:44:04 AM) thpr: So tokens would appear in different places based on what their function is
(9:44:38 AM) thpr: this is setting up the "overflow" (undefined, whatever you'd like to call it) function, so yes, it's probably bad to separate them out
(9:46:14 AM) MotorViper: A bit of clarification on the first tab, does it have other information other than just name which is what you seem to have said.
(9:47:12 AM) thpr: not right now
(9:47:26 AM) thpr: but TYPE would eventually be put there
(9:47:29 AM) thpr: as would other tokens
(9:47:50 AM) MotorViper: its basically a placeholder on the first pass
(9:47:50 AM) thpr: the things that refer to the object itself
(9:47:53 AM) thpr: yes
(9:48:08 AM) thpr: I'm trying to define a set of work that builds the framework
(9:48:13 AM) thpr: but isn't a ton of effort
(9:49:40 AM) MotorViper: worth it though as once you've done that abstracting out the tokens will be possible
(9:51:22 AM) thpr: yea, the point is making it so you can do those 500 ish tasks one at a time
(9:51:46 AM) thpr: and part of the reason James is putting Spring into the code base is so that things with shared structure only need one set of UI code written
(9:51:52 AM) thpr: and the specific parameters defined in XML
(9:52:44 AM) thpr: Make sense?
(9:52:47 AM) thpr: any other questions?
(9:52:51 AM) thpr: or comments?
(9:53:48 AM) James[Code_SB]: I think that approach looks good
(9:54:22 AM) MotorViper: no questions for now, the initial version of the editor looks fairly straightforward
(9:55:07 AM) MotorViper: the main problem will be working out which tokens should go in the second tab
(9:56:01 AM) MotorViper: may have to start with hard coding unless someone knows better
(9:56:37 AM) James[Code_SB]: You should be able to get a list from the token processing code
(9:57:20 AM) James[Code_SB]: That's the gmgen.pluginmgr.PluginLoader that Tom referred to
(9:57:44 AM) thpr: actually you need to initialize the PluginLoader
(9:58:09 AM) thpr: I'll have to find you an example of that
(9:58:16 AM) thpr: and then it initializes the TokenLibrary
(9:58:21 AM) MotorViper: Just looking at the code now
(9:58:29 AM) thpr: in pcgen.rules.persistence
(9:58:43 AM) thpr: you should be able to interact with TokenLibrary for the most part
(10:00:06 AM) thpr: or technically TokenFamily.CURRENT
(10:01:23 AM) MotorViper: Found it so no more questions at the mo, will put any I have on the message board
(10:02:09 AM) thpr: ok
(10:02:22 AM) James[Code_SB]: So next item, facet changes
(10:02:36 AM) James[Code_SB]: Over to you Tom
(10:02:37 AM) thpr: I think that's me
(10:03:16 AM) thpr: A bit in review - we are decomposing PlayerCharacter into individual facets (pockets of function) in order to decrease its size (20K lines)
(10:03:24 AM) thpr: and to extract out set of behavior that are common
(10:03:32 AM) thpr: I've created on the order of 50 facets down that direction
(10:04:07 AM) thpr: These range from reasonably simple (HandsFacet) to moderately complex (ClassFacet)
(10:04:26 AM) thpr: generally, they communicate with each other either through direct references (which we will eventually use Spring to initialize)
(10:04:29 AM) thpr: or through events
(10:04:49 AM) thpr: I have added in some test cases and many of the facets are now commented
(10:05:13 AM) thpr: (I have more test cases written on my laptop, which I will check in the next time I reboot it into linux)
(10:05:54 AM) thpr: Feedback is welcomed on facet structure or if people are not clear on anything
(10:06:17 AM) thpr: goal is effectively full docs and full unit tests for each facet, so we can isolate function and be able to test it
(10:06:23 AM) thpr: (which is a problem in PlayerCharacter today)
(10:06:34 AM) thpr: Two challenges
(10:06:50 AM) thpr: First is abilities - today we have the rebuildAggregateAbilityWorker processing those
(10:07:16 AM) thpr: it will take some work to unwind that, but my priority is to do the CHOOSE work first
(10:07:43 AM) thpr: once that is unwound, PlayerCharacter really starts to fall apart (since the getCDOMObject method can be extracted from PlayerCharacter)
(10:08:05 AM) thpr: so that will be a major event when completed
(10:08:32 AM) thpr: The other challenge we discussed previously and that was in the circular dependent nature of certain items
(10:08:53 AM) thpr: meaning Ability can add a template that can add an Ability, and that can produce loops in the facet dependencies
(10:09:43 AM) thpr: I've realized we can break that (and would break it anyway) by using a formal "addition" system (which eventually would be the addition to the Character graph)
(10:09:51 AM) James[Code_SB]: We will probably need a general solution for that as with the global tags there can be a lot of that sort of circular referencing
(10:10:30 AM) thpr: I think my solution is general enough to work just about everywhere. (I haven't found a counter-case where it doesn't work)
(10:10:37 AM) James[Code_SB]: Excellent
(10:10:46 AM) thpr: basically any time a "native" object (Language, Ability, etc.) is added, the loop is broken
(10:11:15 AM) thpr: so it really comes down to finding the time and getting through the other projects (formula then CHOOSE)
(10:12:09 AM) thpr: questions or comments?
(10:12:50 AM) James[Code_SB]: The facet changes are going well from what I have looked through
(10:16:40 AM) thpr: I think so
(10:19:01 AM) thpr: that's it from me
(10:20:06 AM) James[Code_SB]: ok, Mark did you want to talk about the logging improvements?
(10:20:24 AM) MotorViper: Fine
(10:21:02 AM) MotorViper: The main change I'm making is that I'm moving the parsing specific logging code out of the Logging class
(10:21:27 AM) MotorViper: This has been put into local objects which are returned from the parsing functions
(10:22:07 AM) MotorViper: These objects then know if the parsing has succeeded and if not why not.
(10:22:51 AM) MotorViper: There is a templated version of the object that can be used for parsing functions that return something other than a bool
(10:22:52 AM) thpr: so parse no longer returns a boolean
(10:22:59 AM) MotorViper: That's correct
(10:22:59 AM) thpr: but returns a ParseResult or some such?
(10:23:10 AM) thpr: some interface of some sort
(10:23:34 AM) thpr: how complicated does that make a token?
(10:23:47 AM) MotorViper: Not an interface at the moment but it could be don't think it's necessary
(10:23:48 AM) thpr: have you done a simple token you can show
(10:24:03 AM) thpr: something like HANDS in Race
(10:24:08 AM) thpr: which is basically an integer
(10:24:22 AM) MotorViper: Yes
(10:25:32 AM) MotorViper: Actually I hadn't done that one yet but just have!
(10:26:08 AM) James[Code_SB]: Maybe post that to the dev list (or upload it)
(10:26:28 AM) thpr: it can't be that long...
(10:27:03 AM) thpr: While he's working that, let me chime in on the change from boolean
(10:27:08 AM) MotorViper: I can just paste it in here now if you want
(10:27:15 AM) thpr: yea- I think that's fine
(10:27:26 AM) MotorViper: public ParseReturn parse(LoadContext context, Race race, String value)
(10:27:27 AM) MotorViper: {
(10:27:27 AM) MotorViper: ParseReturn pr = new ParseReturn();
(10:27:27 AM) MotorViper: try
(10:27:27 AM) MotorViper: {
(10:27:27 AM) MotorViper: Integer in = Integer.valueOf(value);
(10:27:28 AM) thpr: I'll come back to the boolean thing after you post it
(10:27:29 AM) MotorViper: if (in.intValue() < 0)
(10:27:31 AM) MotorViper: {
(10:27:33 AM) MotorViper: pr.addErrorMessage(getTokenName() + " must be an integer >= 0");
(10:27:35 AM) MotorViper: return pr;
(10:27:37 AM) MotorViper: }
(10:27:39 AM) MotorViper: context.getObjectContext().put(race, IntegerKey.CREATURE_HANDS, in);
(10:27:41 AM) MotorViper: }
(10:27:43 AM) MotorViper: catch (NumberFormatException nfe)
(10:27:45 AM) MotorViper: {
(10:27:47 AM) MotorViper: pr.addErrorMessage(getTokenName()
(10:27:49 AM) MotorViper: + " expected an integer. Tag must be of the form: "
(10:27:51 AM) MotorViper: + getTokenName() + ":<int>");
(10:27:53 AM) MotorViper: }
(10:27:55 AM) MotorViper: return pr;
(10:27:57 AM) MotorViper: }
(10:28:25 AM) MotorViper: Of course the formatting is better in the editor!
(10:28:26 AM) thpr: A few thoughts
(10:28:41 AM) thpr: first, on the change from boolean - I expect that based on our discussions
(10:28:53 AM) thpr: that's an arch direction as well, for other reasons
(10:29:11 AM) thpr: mainly that I'd like (someday) to extract the common PRExxx parsing
(10:29:36 AM) thpr: which would require the token return something that knows isPrexxxLegal() or some such
(10:29:54 AM) thpr: (and what object to add the Prerequisites to)
(10:30:12 AM) thpr: so if anyone is overly concerned about that change, I think there are reasons to support it
(10:30:25 AM) thpr: comment #2 is that in a success, ParseReturn is empty
(10:30:37 AM) thpr: seems to me we'd want that to be return ParseReturn.SUCCESS
(10:30:40 AM) thpr: which is a static, reused object
(10:31:02 AM) thpr: and failures could return new ParseFailure("message");
(10:31:10 AM) thpr: hence my comment earlier about interfaces
(10:31:37 AM) thpr: I guess that really leads to the question - is there a reason to believe we need more than one message returned?
(10:31:41 AM) James[Code_SB]: It would be nice to see some way of tracking row and column too for future editing extensions
(10:32:04 AM) MotorViper: that was the next part
(10:32:06 AM) thpr: I expect this is step 1 to getting there
(10:32:08 AM) thpr: :)
(10:32:13 AM) James[Code_SB]: Cool
(10:32:44 AM) James[Code_SB]: @Tom, if it is it could be handled as a separate case. That's when you create a new ParseReturn object and add in the errors
(10:33:10 AM) MotorViper: only reason for using addErrorMessage etc. is that it made the updates easier
(10:33:25 AM) James[Code_SB]: I like the idea of having a shortcut to return the normal single error aborts parsing situation
(10:33:50 AM) MotorViper: so do I, I'll add that in now
(10:33:59 AM) thpr: and I'd like to keep the success path from having to construct a new object
(10:34:09 AM) thpr: since in theory the success path is used 99.99999% of the time
(10:34:23 AM) James[Code_SB]: true
(10:35:20 AM) MotorViper: how about the templatised version that wraps an object for parse methods that return something other than bool any comments on that
(10:35:46 AM) thpr: not following
(10:36:13 AM) MotorViper: I'll try and hunt out an example
(10:36:52 AM) thpr: if you're referring to things that aren't CDOMToken
(10:36:56 AM) thpr: you shouldn't have to worry about those
(10:37:08 AM) thpr: but you do apparently need to worry about my spelling (ugh)
(10:37:21 AM) thpr: the non-CDOMToken items are from the GameMode which you don't need to edit
(10:37:48 AM) MotorViper: no I'm thinking about internal methods of tokens
(10:38:12 AM) thpr: like spells
(10:38:12 AM) thpr: SpellsLst
(10:38:21 AM) thpr: which has a finalize method that returns the boolean
(10:38:26 AM) thpr: ?
(10:40:20 AM) thpr: Let me go into comment #3 which kind of ties into this
(10:40:31 AM) thpr: I think there may be a way to do this gradually
(10:40:35 AM) thpr: rather than one huge cutover
(10:40:39 AM) thpr: to alleviate your pain
(10:41:13 AM) thpr: if you go back to the 5.16 branch, or SVN 9294 in the Trunk, and look at pcgen.persistence.lst.PCAlignmentLoader
(10:41:30 AM) thpr: you will notice code that does this:
(10:41:31 AM) thpr: if (context.processToken(alignment, key, value))
(10:41:31 AM) thpr: {
(10:41:31 AM) thpr: context.commit();
(10:41:31 AM) thpr: }
(10:41:31 AM) thpr: else
(10:41:32 AM) thpr: {
(10:41:34 AM) thpr: context.rollback();
(10:41:36 AM) thpr: if (tokenMap.containsKey(key))
(10:41:38 AM) thpr: {
(10:41:40 AM) thpr: PCAlignmentLstToken tok = (PCAlignmentLstToken) tokenMap
(10:41:42 AM) thpr: .get(key);
(10:42:05 AM) thpr: this allowed a token for a PCAlignment to implement EITHER CDOMToken<PCAlignment> or PCAlignmentLstToken
(10:42:25 AM) thpr: if you create another interface (a "peer" to CDOMToken), you can do a gradual conversion in the same way
(10:42:39 AM) thpr: CDOMToken would eventually go away when your conversion was complete
(10:43:03 AM) thpr: (which also give you your progress meter - how many things are still implementing CDOMToken)
(10:43:18 AM) thpr: I'd be happy to help you set that up if you're interested
(10:44:23 AM) thpr: Also lets you do a lot of the implementation work on the base tokens while we can ponder the complex ones on the list or at these meetings
(10:45:31 AM) MotorViper: problem is there are a number of places where processToken is called, I'll have a look at how long it's going to take and let you know if I need to do that
(10:46:13 AM) thpr: there are, but it's not unreasonable if you want to use that method
(10:46:24 AM) thpr: it's what I did in the transition to CDOMToken
(10:47:43 AM) MotorViper: the other thing of course is that going through and doing the whole lot in one go is a big update is that going to be ok?
(10:48:10 AM) thpr: that's part of my concern
(10:48:37 AM) thpr: it's not a problem per se, but it is higher risk than a gradual conversion
(10:48:48 AM) James[Code_SB]: Main risk is either that a) something bad happens to your drive and you lose your work; or b) a change is checked in that causes you to need to do lots of adjusting
(10:49:07 AM) James[Code_SB]: refactoring, which we do a bit is the main cause of risk b
(10:49:43 AM) MotorViper: thought it might be, I was worried about that myself so it looks like this is a good idea
(10:51:51 AM) thpr: I can set up that extra wrapper / converstion setup this weekend
(10:52:08 AM) thpr: if you'd like
(10:52:39 AM) MotorViper: its ok I can do it unless you have a particular way that it should be done
(10:52:59 AM) thpr: all you then
(10:53:22 AM) MotorViper: ok, anything else on this?
(10:53:35 AM) thpr: would be good to see it checked in w/o any token changes, so it's easier to see by itself
(10:53:49 AM) thpr: that's it from me
(10:54:16 AM) MotorViper: I'll do the check in over the weekend, do we check straight into trunk?
(10:54:23 AM) thpr: yep
(10:54:38 AM) thpr: we're in alpha mode now, so we can make changes like that into the Trunk
(10:54:44 AM) James[Code_SB]: So after all the in depth feedback, I was thinking, would ParseResult be a better name than ParseReturn ?
(10:55:25 AM) MotorViper: 50/50 for me, anyone else
(10:56:49 AM) thpr: ParseResult is probably better semantically
(10:56:58 AM) thpr: IMO
(10:57:11 AM) James[Code_SB]: In trying to justify why I prefer ParseResult, it boils down to I find that Result is more descriptive about what is happening. It defines it closer than Return
(10:57:36 AM) MotorViper: true ParseResult it is then
(10:58:48 AM) thpr: Thanks for putting up with all our comments Mark
(10:58:59 AM) James[Code_SB]: Agreed!
(10:59:24 AM) MotorViper: thats ok feedback is always useful and anyway I'm just a newbie :)
(10:59:52 AM) thpr: I think we're about at our 2 hour limit
(11:00:05 AM) James[Code_SB]: So that was the last thing on the agenda - is there anything else to be discussed?
(11:00:10 AM) James[Code_SB]: Yep, pretty much
(11:00:27 AM) thpr: I have two quick comments about next meeting
(11:00:43 AM) James[Code_SB]: Sure
(11:00:49 AM) thpr: one is to have people thinking about dates that may be bad, and to share those with James
(11:01:31 AM) thpr: and two is a proposal for a major topic for next time - output sheets & output tokens. If we want to do that, we should poke Kar to do his homework.
(11:01:58 AM) James[Code_SB]: We would be looking at mid November for the next meeting
(11:02:35 AM) thpr: Note Nov 27 is American Thanksgiving
(11:02:41 AM) thpr: so that's not a doable date
(11:02:57 AM) thpr: er, the 26th is Thanksgiving
(11:03:01 AM) thpr: but the 27th being that Friday
(11:03:10 AM) MotorViper: 6th is only bad date for me
(11:03:48 AM) James[Code_SB]: How about we aim for 13/14 with 20/21 as the fallback
(11:04:42 AM) thpr: sounds good to me
(11:05:55 AM) James[Code_SB]: Right sounds like that's it for the meeting
(11:05:59 AM) James[Code_SB]: Thanks for coming along everyone - great discussion as always