mikaelhg/openblocks

Potential Load problem

taweili opened this issue · 11 comments

I just discovered a bug in the latest openblocks which may cause problem of not being able to load a previous saved file. The problem is cause by the getNextBlockId() introduced in the WorkspaceEnvironment. The number of the nextID there is always being set to 1 when the system started. However, when a saved file are loaded, it will has its own block id that may be conflict with one of the nextID.

I have fixed the problem on my fork https://github.com/taweili/openblocks but this fork also contains some codes specific to ardublock I am working on. I can send you a pull request when I get a chance to clean up my fork. However, I think this should be addressed ASAP, especially for those who are using openblocks in their projects.

By the way, my quick fix is to set the nextId value to the max id value + 1 while loading a document. This can be done at

src/main/java/edu/mit/blocks/workspace/Workspace.java

     public void loadWorkspaceFrom(Element newRoot, Element originalLangRoot) {
        // find max id value from newRoot and set it in WorkspaceEnvironment. 
         if (newRoot != null) {
// ....

Have you already fixed this in your tree, @philippecade?

I think so, please look at 7310667. Looks like we went a different way to correct this though.

I don't think that address the problem. The next block to be initiated still gets its ID from nextBlockId in the WorkspaceEnvironment which is 1 at startup. When you load a file with some block with id 1 in there, we have a conflict of block IDs.

I tried to load a file with a block id set to 1 (I edited manually the file to get that), and it does work fine from my side.

In fact, 7310667 consists in activating the usage of block IDs translation. When a file is load, each block id contained in the xml is associated with a new id, got with the getNextBlockID() method, so the nextBlockID attribute remains consistent with all existing blocks.

This stuff is done in the Blocks.translateLong() method, which already existed before, but which only perfoms the translation in the case its mapping attribute is not null. And this is the case if the importingPage boolean is set to true instead of false in the loadPageFrom() method call while the file is loaded, this is what the 7310667 fix modifies.

Having a look at each call of the following stack should help to observe the relation between the importingPage boolean and the ids translation activation:
edu.mit.blocks.workspace.BlockCanvas.loadSaveString(BlockCanvas.java:477)
edu.mit.blocks.workspace.PageDrawerLoadingUtils.loadPagesAndDrawers(PageDrawerLoadingUtils.java:179)
edu.mit.blocks.workspace.Page.loadPageFrom(Page.java:734)
--> idMapping map only instanciated if importPage is true, and then given as argurment to subcalls until reaching the translateLong() method
edu.mit.blocks.renderable.RenderableBlock.loadBlockNode(RenderableBlock.java:1777)
edu.mit.blocks.codeblocks.Block.loadBlockFrom(Block.java:1436)
edu.mit.blocks.codeblocks.Block.translateLong(Block.java:1554)
--> id translation if the given map is not null

So as said it works fine from my side, but please let me know if you have a precise workflow to make a bug appear.

@taweili, is the issue reproducible with that patch?

Have you come to a consensus on which fix should go into master?

Since there is already existing code that addresses the issue in the library, I think using it would be best instead of adding a quick fix that does the same.

Still waiting for feedback from @taweili

Hi guys,

Sorry for the hiatus. I have been at crunch mode in another project to be delivered today. I will try @philippecade branch this weekend and see if it fix the problem.

Are the IDs even useful, outside of XML saving/loading? It seems like it might be a lot simpler to use real object references instead. Is there some special benefit to the extra lookups?

Hey, you all have bumped into this problem and proposed a solution. The first clean pull request will get in.