[jmarshall]

@grajen3: I think from that one constructor it's pretty clear that this isn't really going to be viable - i.e. you were right, I was wrong . There's just too many hacks (eg you can't just pass a single xml node into the member constructors) to really get it to be nice enough. What's your feeling on that?

As for the loading - I think all you need check is the includes map, though I wonder how the setting of initial states of the controls would work - I *think* it should be OK.

Cheers,
Jonathan

[pieh]

@grajen3: I think from that one constructor it's pretty clear that this isn't really going to be viable - i.e. you were right, I was wrong . There's just too many hacks (eg you can't just pass a single xml node into the member constructors) to really get it to be nice enough. What's your feeling on that?

To be honest, I was hoping You will say that. Moving code to ctors was messy and tricky and in the end - it wasn't so much cleaner after all. I guess that makes us back to this point: github's commit.

Thing that could be changed - is the way of determining if control is focusable at all. I'm thinking about changing ControlMapping in ControlFactory

Code:

typedef struct
{
  const char* name;
  CGUIControl::GUICONTROLTYPES type;
  [color=#880000]bool isFocusable;[/color]
} ControlMapping;

and do proper changes to CGUIControlFactory::IsFocusable(CGUIControl::GUICONTROLTYPES type)

As for the loading - I think all you need check is the includes map, though I wonder how the setting of initial states of the controls would work - I *think* it should be OK.

From quick tour around related code it seems that AllocResources() / FreeResources() don't do anything fancy.

From fast checking on it - it runs pretty good, higher memory usage after running it for some time (of course they it's not getting higher and higher endless) - but this was to be expected. I think that one extra thing to do would be to free resources of child controls (mainly image textures) and of course check that include conditions - I'll craft something tomorrow.

[jmarshall]

Struct sounds sane - go for it.

[pieh]

Here's improved xml parsing:
https://github.com/pieh/xbmc/commits/xml...litting_v2

And here's experimental branch - window doesn't destroy its controls on WindowDeinit. Therefore we don't have load xml file and create all controls again when reusing same window - that is ofcourse if none of the conditions used to resolve conditional includes changed it's value
https://github.com/pieh/xbmc/commits/don...dow_struct

Next I would like to check is if loading items from database wasn't blocking window loading - now we wait until all items are prepared then window is showing. What I would like to try is to move loading data from database to background thread and notify window on progress - so window would load and show immediately and items would start showing up in list as they are prepared.

I did some timing tests when debugging: GetMoviesNav - main SQL query was taking 75ms with ~200 movies and iteration over results and assigning values to FileItems (+ some additional streamdetails query) was taking 450ms. So that means user have to wait over 500ms to get into navigation. In my idea first items would start showing after 75ms so this could be nice improvement in end user experience.

Any thoughts on this?

[jmarshall]

Ok, to solve this you'd need to consider:

1. Which views should be available for a particular view? Ideally you need to know this before fetching the folder, as otherwise the window may fill without the appropriate views being present. Depending what visibility conditions the skinner is using, the UI may look odd if nothing is available to it. In general, you won't know this before retrieving this listing, so you may have to default things on window load (default values that the CFileItemList has may suffice already). ***

2. I'm sure that in several places we assume that CGUIMediaWindow::Update() is synchronous. You'd have to check carefully where it's being used to make sure that it won't matter if it wasn't synchronous.

3. The existing CGUIMediaWindow::Update routine (and in particular it's overrides) would have to be split up so that the meat (GetDirectory) can be made asynchronous. Maybe something like OnUpdateStart/OnUpdate (non-virtual, base class only)/OnUpdateEnd or some such may be suitable. Basically you want to ensure that you can split the program flow up.

4. Then move the non-virtual OnUpdate to fire off a job. You'll have to protect any members that the job touches via a critical section (or better, fire a thread message to tell the window the update is complete).

5. Extra for experts: Look at changing the CDirectory::GetDirectory api to take in a callback function so that progress can be reported, and cancellation can occur. A progress control could be added to the window classes to deal with this, and (potentially) you could update the UI as you go.

Cheers,
Jonathan

[pieh]

@jmarshall:
I have read Your comments on my git brances and will do suggested code changes. Just 1 question - reseting git tree and commiting modified code will delete also Your comments, right? Should I stash them somewhere?

---

Ad 5. As for making filesystem async - I think I would do it like this:

Code:

class IDirectoryCallback
{
public:
  virtual void onProgress(const CFileItemList& list_of_new_items, const int length_of_all_items = -1); // <- this could be called at some predefined frequency - if onProgress was called 50 ms or less ago - stash items, otherwise - call onProgress with stashed items
  virtual bool onFinish(const CFileItemList& complete_list);
  virtual void onCancel(); - if getting directory was canceled
};

Then add CDirectory::GetDirectoryAsync(strPath, items, IDirectoryCallback object). It will run background thread that will simply run normal GetDirectory method and report onFinish when GetDirectory will finish.

Next allow GetDirectory to accept IDirectoryCallback as a parameter (NULL if not specified) and if this paramater is set - use it to report progress only.

Here I would propably change one of the database method to report progress and will continue on changing CGUIMediaWindow to make use of it.

Ad 1. I will have to take a look at where in code ContentType is set and think more about it. If I'm not mistaken currently content is set in CGUIWindowMusicNav/CGUIWindowVideoNav::GetDirectory() . I was thinking that virtual filesystem should setContent of retrieved items? Now we retrieve items, check what virtual node we was using and then based on that virtual node we set content. If GetDirectory would set content of items there wouldn't be much we should be worry about, right?

Ad 2. Surely this will have to be investigated.

Ad 3,4. OnUpdateStart to start async filesystem requests, OnUpdateEnd would be called when filesystem will notify onFinish and handling onProgress in meantime.

[jmarshall]

Don't worry about dropping my comments unless you need them - I'll read through the whole commit series again anyway. Just commit your changes, interactively rebase to tidy them up as needed and push (force).

Cheers,
Jonathan

[pieh]

After fair amount of break, I'm back to bug about improvements:

• Keeping window objects in memory when deiniting window to avoid loading xml file again when going back to window we were already before: https://github.com/pieh/xbmc/commits/kee...ow_objects
  
  I've done some improvments since my last attempt - all the magic essentialy happen now in CGUIWindow::AllocResources() method. I've reused bool CGUIWindow::m_loadOnDemand member that was hardcoded to be true for windows (and thus forcing freeing resources when deiniting window). This is how some dialogs (DialogKaiToast, DialogBusy, etc) are kept in memory (at least I believe so).
  
  Also added failsafe mechanism to force unloading if we are running out of memory - if free memory drops below threshold value (by default 200 MB - adjustable via advancedsetting, default value is also subject for discussion) window is freeing resources when deiniting
  
• Selective parsing of .xml control tags (reading only those xml attributes that are proper for given control type): https://github.com/pieh/xbmc/commits/spl...o_controls
  
  

[jmarshall]

Thanks

Do you think these are good enough shape to do a pull request as-is, or would you prefer another round of comment/cleanup before the pull request?

Cheers,
Jonathan

[pieh]

I just wanted to let You know that I didn't disapperead with my ideas. I believe there is still some improvement possible:

For xml->controls conversion:
current solution results in code duplication - for example:

Code:

CLabelInfo labelInfo;
    GetLabelInfo(pControlNode, labelInfo);

is copied in all control types that uses text rendering attributes

What I was thinking is to define some interfaces that controls could inherit (f.e. ITextureAttribute, IFocusableAttributes, ITextRenderingAttributes - naming would need some thinking about obviously). In CGUIControlFactory::Create use dynamic_cast to test if class inherits ITextureAttribute - if it does - parse xml for <texture> attribute and set it in control using method inherited from ITextureAttribute. This would abstract process of control creation - CGUIControlFactory will not need to know that Control X have Y,Z attributes - it would be enough that Control X know that it have Y,Z attributes. Of course this would require changes to control constructors. What's more this could get help in getting rid of heap of temporary variables in CGUIControlFactory::Create.

Here's some example:

Code:

class ITextRenderingAttributes
{
protected:
  ITextRenderingAttributes() { } //only for inheriting purposes
  CLabelInfo m_labelInfo;        //structure containing text rendering attributes
public:
  CLabelInfo& GetLabelInfo();    //return m_labelInfo
};

Controls that render text (buttons, labels, etc) would inherit it (it will need removing of member m_labelInfo in these controls)

In CGUIControlFactory::Create (or even in CGUIControlFactory::GetLabelInfo itself):

Code:

ITextRenderingAttributes* textRendering = dynamic_cast<ITextRenderingAttributes*>( control);
if (textRendering) // if control inherit ITextRenderingAttributes then load labelinfo
{
  GetLabelInfo(pControlNode, textRendering->GetLabelInfo());
}

Question here is - is it worth to do extra work for it? Is dynamic_cast ok for this? Are there better ways to abstract process of control creation?

Sorry I'm such pain in the ..., I may have (some) knowledge abound coding, but I'm lacking the wisdom and experience

---

As for the other branch: Keeping window objects in memory, after making changes You have stated in comments and adding 'const' and '&' here and there I think it will be ready to request pulling

[jmarshall]

Hmm, I'm not sure whether it's worth it to be honest - you're adding overhead to each access of m_labelInfo at the control level (via the additional inheritance) which could become an issue on CPU-starved platforms.

I don't think the code dupe is all that bad - sure, it'd be nice if it wasn't there, but it's equally nice to keep things simple.

Cheers,
Jonathan