Understanding the Code

[Fidcal]

While you're looking at the code, remove the space blocker from the inventory name. Inventory names can have spaces. It's the xdata def which can't.

[greebo]

That's already done: http://bugs.angua.at/view.php?id=2388

[Fidcal]

Forgot about that tracker entry. But that has fixed in 1.5 and it's still not working in pre 1.5 ?? Maybe just missed it. I seem to recall a release 19 December and that fix is 21 December? Just guessing.

[Fidcal]

StiFu: another small glitch while you're looking at this code:

 

The editor does not retrieve the xdata name from the entity if it exists. For example, I create a readable, give it a name, an inventory name, and an xdata name (no need to I know at this point if I'm going to use the editor.) I've not actually created any xdata manually. I now open the editor and choose a gui and type some text but when I go to save it says I must give the xdata a name so I have to type it in again.

 

It must retrieve that name the second time I open it in the editor so the function must already be there but it's just when creating a new one it doesn't.

[STiFU]

Alright, I think I fixed the issues now (Issue #2584). The guiSelector had two issues with the notebook. One of the fixes is a rather dirty one since I simply delete the notebook object to prevent it from switching pages when destroying the window. Is this practice ok, Greebo? I added the issue to target version 1.5.0, ok?

 

@Fidcal: That is more of a featurerequest, but it should be easy to do. I'll have a look.

[STiFU]

Here is the thing. Currently the importer does not differ between "import failed due to faulty xdata definition" and "import failed due to definition not found" and only in the latter case, it would be ok to use the predefined xdata name. So implementing the requested feature would require a major code rearrangement, which I am not willing to do, seeing that it's such a minor tweak.

 

However, I could just make it keep the xdata name generally regardless of the cause of import failure. This would mean that upon failing to import a definition, a definition with the same name would be created, but the user wouldn't be notified about this duplicate, because (again) the importer failed to import the first definition. The xdata import summary in the tools menu does show that there were multiple definitions. Maybe I can do something, so that the user is at least warned about this, but I am out of time for today.

[greebo]

Alright, I think I fixed the issues now (Issue #2584). The guiSelector had two issues with the notebook. One of the fixes is a rather dirty one since I simply delete the notebook object to prevent it from switching pages when destroying the window. Is this practice ok, Greebo? I added the issue to target version 1.5.0, ok?

I haven't looked at the changes yet - do I assume correctly that there is a callback/signal fired on window destruction? If such a signal handler is the problem, the solution would be to disconnect the handler in the _preDestroy() event of the dialog.

 

Not that your solution is particularly bad or something, whatever works it's most probably fine, unless you've got the feeling that your solution will break sooner than later.

 

Thanks for looking into it in any case.

[STiFU]

No problem. It felt good to finally do something on DR again. And bugfixing is not such a huge task compared to rearranging the whole importer and the gui, as it used to be my plan. Is there actually need for that plan to be realized? Rewriting the importer code to support all possible xdata definitions won't be hard, but adapting the Readable Editor Gui to it while maintaining a good comforting user-workflow will be a messy task.

 

Anyway, your assumption is correct and your proposal would probably be the most elegant solution, but seeing that the notebook will never be accessed after the window was destroyed, I guess we're good with the current approach.

[greebo]

As long as the readable editor does its job we don't need to reorganise it just for the sake of it. Time will tell when the first roadblock will be hit, so let's leave it at that for now.

[STiFU]

Ok, I committed my work on Fidcal's feature request. A predefined "xdata_contents" property now sticks when launching the editor. As I pointed out earlier, this can result in a problem if there was a definition with the same name that failed to import. Here is a scenario that shows how the editor behaves in that case:

• Define xdata_contents on an entity before launching the editor. A faulty definition with that name is already defined in a file.
  
• Launch the Readable Editor
  
• Readable Editor shows an error message, saying that the importer failed, but the suggested xdata name is still used for the new xdata definition.
  
• Upon saving a duplicated definition will be created.
  
• When launching the Readable Editor the next time, the importer will successfully import the newly created xdata definition, but will show a warning saying that there were problems during import and the user is asked whether the xdata import summary should be displayed.
  

 

Is this behavior ok? I had to make showXdImportSummary() for this to work. If this implementation is acceptable, should it be included in 1.5.0? If this implementation is not acceptable, we should just revert the commit, as a proper solution will require a pretty deep rearrangement of some of the class-methods, which again would require a lot of testing etc. and I am not willing to do that for such a small tweak.