Aug 2011 GUI Overhaul

[Chris H.]

I have not had a chance to link about it but I like the idea with the list of cards that you can select with the picture and the card detail panel.

[Doublestrike]

How do I assign myself to an issue on Mantis? I'd like to take a stab at http://cardforge.org/bugz/view.php?id=155 but I can't seem to find where to volunteer. (Access level? Still a 'reporter')

@Braids - I know this is your pet, hopefully you don't mind a little hand-holding since I'm still learning the ropes? I'd like my Jeweled Torque to remain un-kicked.

Alternatively, since there's a very real possibility of my taking more than 48 hours to finish this change, perhaps there's a way I could duplicate or complete the work for whoever takes on this task...

[Braids]

How do I assign myself to an issue on Mantis? I'd like to take a stab at http://cardforge.org/bugz/view.php?id=155 but I can't seem to find where to volunteer. (Access level? Still a 'reporter')

@Braids - I know this is your pet, hopefully you don't mind a little hand-holding since I'm still learning the ropes? I'd like my Jeweled Torque to remain un-kicked.

Alternatively, since there's a very real possibility of my taking more than 48 hours to finish this change, perhaps there's a way I could duplicate or complete the work for whoever takes on this task...

nobody has really said whether we're going through with it. please see http://www.slightlymagic.net/forum/viewtopic.php?f=52&t=5204&start=15#p68526

i guess you could take a shot at it. i would hope it to be {extend?} a JComponent or such that fills up as much space horizontally as its container will allow. maybe it could have a getModel() method that returns another class that extends net.slightlymagic.braids.util.progress_monitor.BaseProgressMonitor, which is how external forces could increment the units processed.

mind your threads. only code running in an event dispatch thread is allowed to update the GUI. other code should run in a different thread if possible.

i don't think you're under a 48 hour time limit.

you {Doublestrike} now have developer access in Mantis. below the item's main description, you can use the Edit button or the Assign To buttons to assign yourself the action . . .

[friarsol]

i don't think you're under a 48 hour time limit.

Definitely not. Code takes as much time as it takes. As long as it's clear that you are handling that portion of the code, noone else should be stepping on your toes.

[Doublestrike]

i would hope it to be {extend?} a JComponent or such that fills up as much space horizontally as its container will allow. maybe it could have a getModel() method that returns another class that extends net.slightlymagic.braids.util.progress_monitor.BaseProgressMonitor, which is how external forces could increment the units processed.

mind your threads. only code running in an event dispatch thread is allowed to update the GUI. other code should run in a different thread if possible.

Was thinking the same re: JComponent. Dispatcher threads noted. Even if the splash screen isn't used, it'll be a good place to have my first go. Will do my best, thanks for the tips.

[Braids]

Was thinking the same re: JComponent. Dispatcher threads noted. Even if the splash screen isn't used, it'll be a good place to have my first go. Will do my best, thanks for the tips.

well, i should have rephrased that. the progress bar should take up the entire width granted by the container. it shouldn't demand more than it needs. its minimum width? i'll leave that an exercise to you.

[friarsol]

I just played a few quest games and it seems like the Card Picture in the new Quest Won Card screen isn't displayed (but the CardDetailPanel is). Is this not completed yet? I have all the LQ Set pictures downloaded. I've been grabbing Legacy packs if that makes a difference.

[Doublestrike]

I've finished integrating the progress bar into the splash screen, but having some temporary stupidity with SVN (see my post in "How to Get Started" thread). Once that's figured out, will test and release.

@Braids - the interfaces and base class for the progress bar are great tools for future GUI work. I'd like to put them in a forge.view.util package for all progress bars required in the project. Proposed structure:

Code: Select all

forge.view.util
 |- ProgressBar_Base.java
 |- ProgressBar_DialogsMultiple.java
 |- ProgressBar_DialogsSingle.java
 |- ProgressBar_Embedded.java
 |- ProgressBar_Interface.java
 |- ProgressBar_StdErr.java


Much of this comes from ...braids.util.progress_monitor which looks pretty much ready to be evolved/integrated. I've got this mostly working in my local workspace. Any major issues with this? Will commit soon so you can see it working with the splash frame (which uses an embedded progress bar instance).

Update
Have put in the splash frame change using an embedded progress bar. If is OK will move on to update the dialog-based progress bars (and the stderr also). For now, I'm off to learn PMD, Checkstyle, TestNG, and FindBugs.

[Max mtg]

I just played a few quest games and it seems like the Card Picture in the new Quest Won Card screen isn't displayed (but the CardDetailPanel is). Is this not completed yet? I have all the LQ Set pictures downloaded. I've been grabbing Legacy packs if that makes a difference.

I've commited a fix for it. Please check if it started working.

[friarsol]

I just played a few quest games and it seems like the Card Picture in the new Quest Won Card screen isn't displayed (but the CardDetailPanel is). Is this not completed yet? I have all the LQ Set pictures downloaded. I've been grabbing Legacy packs if that makes a difference.

I've commited a fix for it. Please check if it started working.

Yep it worked, although you should probably be using the Card.getMostRecentSet() not Card.getRandomSet() since if you grab a Standard pack but it shows a Revised Plains it might look weird.

[Braids]

@Braids - the interfaces and base class for the progress bar are great tools for future GUI work. I'd like to put them in a forge.view.util package for all progress bars required in the project. Proposed structure:

Code: Select all

forge.view.util
 |- ProgressBar_Base.java
 |- ProgressBar_DialogsMultiple.java
. . .


1. class names should not have underscores in them. both CheckStyle and PMD will complain about this.
2. were you not able to reuse any code in net.slightlymagic.braids.progress_monitor via composition {having a member which is a BaseProgressMonitor} or inheritance {extends}? it looks like you copied and pasted a lot, which you shouldn't have needed to do. was there a problem in extending BaseProgressMonitor and/or adding a setModel method to your class that extends JComponent?
3. if your class is specific to swing, it should now go somewhere in the forge.view.swing package hierarchy.

Much of this comes from ...braids.util.progress_monitor which looks pretty much ready to be evolved/integrated.

integrated? braids.util.progress_monitor? are you talking about moving the package's contents? i fail to see what you mean by {evolved/integrated}. i am concerned you are trying to change or move something that should be neither.

edit: the main reason the net.slightlymagic.braids package heirarchy exists is because all code in that hierarchy is dual licensed, GPL and Apache 2.0. this allows me (or anyone else) to use that code in a commercial app.

[Max mtg]

Yep it worked, although you should probably be using the Card.getMostRecentSet() not Card.getRandomSet() since if you grab a Standard pack but it shows a Revised Plains it might look weird.

I'm actually busy at a different area of forge.
If you make that change by yourself, I would mot mind at all

[Doublestrike]

Thanks for the feedback, Braids.

1. class names should not have underscores in them. both CheckStyle and PMD will complain about this.

Changed. Am getting into CheckStyle and PMD today.

were you not able to reuse any code in net.slightlymagic.braids.progress_monitor

Yes, it's the same code, with a less specific name. The BraidsProgressMonitor and BaseProgressMonitor are now ProgressBarInterface and ProgressBarBase (base class implements interface, extended as described below).

The base class is extended as you said, to four instances of progress bar utility: stderr, embedded, single dialog, and multiple dialog (each phase opens a new dialog). The splash frame code was modified to include an instance of the embedded progress bar utility. (Currently that's the only child class written.)

The old folder hierarchy has not been changed.

i fail to see what you mean by {evolved/integrated}.

It looks like the interface and base progress bars could be used as utilities for the new GUI in various places - splash, download, update check, etc - under a naming structure consistent with the project. This is why I moved them into a "view.util" area to go with other GUI utilities that will come later.

if your class is specific to swing, it should now go somewhere in the forge.view.swing package hierarchy.

Renamed to forge.view.swing.utils.

[Braids]

ok, important fact. you created something that works quite well. from the user perspective, it looks really nice.

but looking at ProgressBarEmbedded, i can see that you made some of the same mistakes i myself made with MultiPhaseProgressMonitorWithETA.

we can accomplish two important objectives by changing your implementation: first, separate the model from the view. have the view observe the model {somehow} and be notified when the model changes so that the view can change. second, reuse code from net.slightlymagic.braids.progress_monitor instead of copying and pasting {copypasta} into classes with poor model-view separation. i think you can do this with 2 major classes, and possibly 1 or 2 minor ones for observation and/or a Runnable. it would be best to create a class, say, MultiPhaseProgressBar, which extends JProgressBar. Add to MultiPhaseProgressBar a field of type BraidsProgressMonitor called model, and add a getter and setter method for the model. Create a new subclass of BaseProgressMonitor which is likewise associated by a field with a MultiPhaseProgressBar. When one of the model's mutative {state changing} methods gets called, call the same method on super, then have it notify its associated MultiPhaseProgressBar on the event dispatch thread. you can do this with SwingUtilities.invokeLater, or possibly with an observer relationship, or something involving an ActionListener. {I'm not really sure which of the 3 to use.} The MultiPhaseProgressBar can then read the new values from the model and update itself accordingly from inside the event dispatch thread.

this accomplishes your goals with a lot less code, and a lot less copypasta. it's cleaner and much easier to maintain.

i know it may be difficult for you to rewrite something that works just fine, but i think these changes are important. we need to minimize our technical debt and avoid code bloat. i hope you understand.

[Doublestrike]

Great - thanks for the feedback.

I have no problem at all with rewriting if you are OK with having a look and telling me what I'm doing wrong. I'll have a look at what you said and update accordingly with special attention to MVC.

(There's a few hiccups in my implementation anyway so I'd be rewriting regardless.)