Improve Grid2 Options #59


  • Fixed
  • Enhancment
Closed
Assigned to _ForgeUser117147
  • _ForgeUser1990418 created this issue Jan 5, 2010

    I've done some work to tidy up Grid2 Options. Specifically:
    - Main the 1st tier of options to within the blizzard sub option UI.
    - Changed the indicators page to be tab-based
    - Other tweaks to make the whole thing work.

    I'll attach a screenshot and a patch file. Let me know your thoughts. The only possible downside is I've abandoned (in places) the Grid2OptionsCode AddElementGroup / AddElement API. Personally I think that part of the API makes assumptions that are too rigid about how you'd configure settings and doesn't really provide much benefit. The logical conclusion of this approach is that Indicators / Statuses / etc all have their own option generating scripts and the code that happens to be common moves to a Grid2OptionsUtils package - does this seem a reasonable approach to you? The strongest advantage to this approach is that it allows the IndicatorOptions (say) to be updated without being concerned about all the other options.

    I'm not sure if I have the time / inclination to work on this further. If I were to continue I'd make the Indicator options based on lists rather than the current setup with checkboxes and +/- buttons.

  • _ForgeUser1990418 added the tags New Enhancment Jan 5, 2010
  • _ForgeUser1990418 added an attachment Grid2Options1.jpg Jan 5, 2010

    Grid2Options1.jpg

  • _ForgeUser1990418 added an attachment Jan 5, 2010
    Attachment was deleted Nov 8, 2016
  • _ForgeUser1990418 added an attachment Jan 5, 2010
    Attachment was deleted Nov 8, 2016
  • _ForgeUser1990418 added an attachment Grid2Settings-Ticket59.patch Jan 5, 2010

    Grid2Settings-Ticket59.patch

  • _ForgeUser1990418 added an attachment Jan 5, 2010
    Attachment was deleted Nov 8, 2016
  • _ForgeUser1990418 edited description Jan 5, 2010
  • _ForgeUser1990418 posted a comment Jan 5, 2010

    Fixed some bugs. Heh.

  • _ForgeUser117147 posted a comment Jan 6, 2010

    There are part of this patch that I don't like, mainly this:

    +	--so feed in a dummy
    +	Grid2Options:MakeOptions()
    

    and all the changes that it implies later.

    Why did you feel the need to add this code ?

    Other than that, I need to check in-game, but it looks like an improvement to the original option layout, Thanks.

  • _ForgeUser1990418 posted a comment Jan 6, 2010

    Yeah I figured it might be debatable in places, but then you say yourself "The grid configuration needs a complete overhaul" (http://forums.wowace.com/showthread.php?t=12559) so I thought I'd make a start.

    That particular piece of code is to force all the constant parts of the configuration to be created so that they can be registered once with the blizzard menu. The alternative would be to have MakeOptions dynamically register with the blizzard menu each time a new category of options appeared. But since MakeOptions is called many times the options would need to keep track of what had been registered with the blizzard menu already.

    In my mind it's cleaner for the all the static parts of the options to be created on first intialisation and then have makeoptions only affect the dynamic parts. Admittedly the empty Makeoptions() call is a lazy way of doing this, but at least it's a step in the right direction that can be improved upon.

    [My long-term aim is to improve frame configuration and I can't do that when 1/2 the options panel is filled with a list box of other things to configure - hence the raison d'etre for this patch]

  • _ForgeUser117147 posted a comment Jan 26, 2010

    Did you try to package your branch ? I'd like to test in game to see the result

  • _ForgeUser117147 removed a tag New Jan 26, 2010
  • _ForgeUser117147 added a tag Waiting Jan 26, 2010
  • _ForgeUser1990418 posted a comment Feb 1, 2010

    Not yet. The problem is you keep on making changes to trunk which I have to merge in :D

    I'll see if I can tidy it up enough to bring it to a stable state. Might have to wait until the weekend though.

  • _ForgeUser1990418 removed a tag Waiting Feb 1, 2010
  • _ForgeUser1990418 added a tag Replied Feb 1, 2010
  • _ForgeUser65911 posted a comment Feb 6, 2010

    I like the idea of moving the top layer objects to the Addons tab panel. We should definitely do that. I am not sure I like the tabbed thing on the right without trying it.

    There have been quite a few changes with the layered options now checked in...

  • _ForgeUser1990418 posted a comment Feb 7, 2010

    Okay branch revision 325 contains all the up-to-date options changes. However this is from the last beta release. It should be stable so let me know what you think.

    My aim with this change is simply to improve the options from where they are now rather than provide a definitive answer for how options should be handled. There's still plenty of room for improvement!

    Next I'll try and merge in the layered options stuff and see how it goes.

  • _ForgeUser1990418 posted a comment Feb 7, 2010

    Okay I've merged in up to r348. It's good to go!

    If you could merge it into the main branch that would be really nice as constantly maintaining this branch is a little cumbersome :P

  • _ForgeUser65911 posted a comment Feb 9, 2010

    I am trying to get a picture of the changes but with the merged code i mostly see the layer stuff. The toc thing is weird though. Why does Grid2.toc include all the options files?

  • _ForgeUser117147 posted a comment Feb 9, 2010

    Probably to allow the SVN working copy to successfully load directly from the wow client.

  • _ForgeUser1990418 posted a comment Feb 9, 2010

    Indeed. I couldn't find a better way of getting the options addon to load directly from the SVN working copy. How do you guys deal with this?

  • _ForgeUser65911 posted a comment Feb 9, 2010

    It already works. I develop on and play using the svn version. All the options are lod. Each plugin runtime deals with loading options, same as Grid2 handles loading Grid2Options as necessary.

    LibDBLayers:LoadOptions and LibDBLayers:UpgradeDefaults has the code for it.

    LoadOptions checks if the runtime versions match internal code versions passed to it and loads an options mod if it is out of date.

    Later UpgradeDefaults does the same thing, calling out of date options so they can upgrade.

    Finally, everything is flattened for use.

    If nothing is out of date, then the upgrade and flatten steps do not happen.

    So simply place the svn version into your Addon folder. Then copy out and rename the included Alert and RaidDebuffs and Options folders so they are in the Addons folder as well.

    Just make sure you make changes to these folders and not the included ones since those will have no in game effect.

  • _ForgeUser1990418 posted a comment Feb 14, 2010

    Guys: Are you interested in merging this branch? I don't really want to maintain it forever, especially if you're changing the options code. Now is a pretty good time to merge it in anyway since you've been doing a lot of changes.

  • _ForgeUser65911 posted a comment Feb 17, 2010

    I am interested in the very top level stuff where the top level list gets moved to the left box in the blizz options, as well as the tabbing. There are many things I do not like about the rest so they need to be done separately and afterwards. For instance the multilist drop down to select statuses is just not going to cut it. How would you provide tri-state feedback for it for example? People have enough issues ctrl-clicking as it is.


    Edited Feb 18, 2010
  • _ForgeUser65911 posted a comment Feb 21, 2010

    Or we can start by converting to tabs first?

  • _ForgeUser1990418 posted a comment Feb 22, 2010

    Here's how I see it:
     - Options need some work done on them (Jerry has said as much)
     - Nobody is likely to do major options rewriting anytime soon.
     - My options might not be perfect but at least they are an improvement.

    The multiselect for indicator-statuses that you mention is basically a poor man's list. I had a look at AceGUIWidgets but they don't provide a list, so what I have done is use multi-select but limit it to single selection and make it behave like a list. Yes it's a bit hacky, but so is the current implementation, and mine requires a lot less real-estate. Did you give it a go? I think it's better what we have now.

    Re: "How would I provide tri-state feedback?" well I suspect if we require tri-state feedback we've made the whole thing waaaay too complicated! - but let's worry about that problem when it occurs?

    I'm not claiming it's perfect but I believe it's an improvement (see my points above). But anyhow, it's up to you - you're welcome to take whichever parts you wish. Let me know :)


    Edited Feb 22, 2010
  • _ForgeUser65911 posted a comment Feb 22, 2010

    The layer stuff requires tri-state settings, its kind of the whole point. Checkboxes already have it. The other widgets require some kind of solution as well. You need a way to "delete the custom setting at this layer and use the parent layer". Even if this is done by setting it to the same value as the parent which causes it to nil out it would be better with feedback. If it is nil, it would be good to see what the inherited value is. Etc.

    So go ahead and check in the stuff for the top level moving into the left tab, and convert to tabs for the right hand side. These are clearly improvements.

    After that we can deal with the other issues you want to address individually.

  • _ForgeUser1990418 posted a comment Feb 28, 2010

    Ok I've merged this back into trunk. We can discuss anything left over in a separate thread. Feel free to close the ticket and the branch.

  • _ForgeUser65911 posted a comment Feb 28, 2010

    Excellent, it looks good.

    The branch belongs to you. Only you can dispose of it.


    Edited Feb 28, 2010
  • _ForgeUser65911 removed a tag Replied Feb 28, 2010
  • _ForgeUser65911 added a tag Fixed Feb 28, 2010
  • _ForgeUser65911 closed issue Feb 28, 2010

To post a comment, please login or register a new account.