rgriebl / brickstore

An offline BrickLink inventory management tool.
https://www.brickstore.dev/
GNU General Public License v3.0
116 stars 26 forks source link

Bricksync fails to load bsx file #51

Closed BrickSouth closed 3 years ago

BrickSouth commented 3 years ago

In Bricksync, when I tried to merge or sort a bsx file I saved from BrickStore, I got the error "We failed to load a BSX file at path "filename.bsx". The same file saved from BrickStock worked normally.

ZZJHONS commented 3 years ago

Have you opened the log file of BrickSync to check if in there it says something more?

BrickSouth commented 3 years ago

The log file just says the same thing as the command line. Do you mean the error files it creates when there is an error? It did not make one because I don't think it's necessarily a Bricksync error, just that it isn't recognizing the bsx file for some reason.

paramecie commented 3 years ago

Probably because of this tag: [Inventory Currency="USD"] (sorry but "<" ">" are replaced for security here, please understand "<" and ">")

Temporarily, edit the BSX file with Notepad or Notepad++ (better) and only leave [Inventory]? It's in the 4th line of the file. Please try this?

Simply use CTRL+H to replace ' Currency="USD"' (space before and your currency of course), to be replaced by nothing, or your mouse, select and delete, then save.

Robert, currency could be written above as a specific tag which would be ignored? Like [CURRENCY]EUR[/CURRENCY] Or maybe as a [!CURRENCY] ?

paramecie commented 3 years ago

I made you my first GIF, yes yes :-) Sorry, it loops at then end, ignore this: open the file, delete, save and quit. I'll change the loop setting in my recorder...

Remove with Notepad

Note that this will make BrickStock loose your currency, so next time you'll have to set it up again - but I guess for USD it's by default, so it'll apply USD again, to be removed of course again...

rgriebl commented 3 years ago

And then there's another tool which would happily ignore the Currency attribute, but chokes on an additional tag. I did add the currency via a tag for exactly this reason, because attributes tend get ignore most of the time when parsing XML.

paramecie commented 3 years ago

And comment "[!--' or "[![CDATA["... aren't ignored? Ah... but maybe ignored by BrickStore also :O)

ZZJHONS commented 3 years ago

If this is hard to fix, maybe add a save option without currency? Compatibility with BrickSync is something needed by a lot of stores..

PD: Maybe someone can edit the BrickSync source code instead... https://github.com/zzjhons/Bricksync

paramecie commented 3 years ago

is something needed by a lot of stores..

Don't exagerate, maybe 50 stores have 2 stores on 2 market places; but there are 10 000+ stores and half a million buyers would could use BrickStore on BrickLink ;-)

paramecie commented 3 years ago

@rgriebl One solution would be a specific Export...

image

Like Export "BrickStore without Currency"

The file "MyInventory.bsx" could be named the same with a postfix like "MyInventory_[NoCurr].bsx" ? And then of course it wouldn't have the Currency tag...

rgriebl commented 3 years ago

I took a look at Bricksync's source code ... The mad man parses XML using hand-crafted pattern matching in C. That's a bit too hardcore in 2021 for my taste :) Your best bet would really be to ask the author about adding support (i.e. ignore) for the new attribute.

paramecie commented 3 years ago

Your best bet would really be to ask the author about adding support (i.e. ignore) for the new attribute.

He'll ignore the demand, I guess it's what you meant ;-)

paramecie commented 3 years ago

And then there's another tool which would happily ignore the Currency attribute, but chokes on an additional tag. I did add the currency via a tag for exactly this reason, because attributes tend get ignore most of the time when parsing XML.

That's the point, it should be ignored.

People should be responsible to set the proper values in the Currency that BrickSync expects (settings)?

Then, an attribute would work?

[?xml version="1.0" encoding="UTF-8"?] [!DOCTYPE BrickStoreXML] [BrickStoreXML] --[Inventory] ----[Item] ----[/Item] --[/Inventory] --[Currency] ----[Token]USD[/Token] --[/Currency] [/BrickStoreXML]

rgriebl commented 3 years ago

What I meant to say is this: given the way the Bricksync parser is implemented, it is just a matter of time until it'll break the next time when adding a new feature to BrickStore. I wouldn't have a problem with sending a patch to the Bricksync's author when something like this happens, but that XML parser code is a mess.

paramecie commented 3 years ago

Then, the only option I can see is like the one I proposed, an Export or a Save as... to the ancient format, without the Currency tag.

Which could be helpful if you add new features in the new files, you've still a way to export as the old, immutable basic format.

rgriebl commented 3 years ago

Why is "contacting the author and asking if he could adapt" not an option?

paramecie commented 3 years ago

Because people say developpers are lazy, so he won't ;-)

Seriously: some may still use old-BrickStore, some will still use BrickStock (which I didn't tested if they accept the new BSX).

And you may implement new features in the future.

And you don't know the tens of proprietary private tools that uses BSX files - like mine for example, which accept the new format - but you don't know the others.

Ah, BTW did you test a BrickLink import?

Having a "compatibility format" is a pain for you now, but can be at your honor?

As you think, I don't use BrickSync, and have no problem.

paramecie commented 3 years ago

If this is hard to fix, maybe add a save option without currency? Compatibility with BrickSync is something needed by a lot of stores..

Sorry I didn read well enough what you wrote. Yes, exactly this I proposed too. But then to implement I don't know what can be done, a Save with an option, an Export ? Extension .bsx should remain the same in all cases, that's for sure.

ZZJHONS commented 3 years ago

Having a legacy export would be great as what @paramecie suggests, if you add something more in the future that needs Bricklink fixing in its upload, you could be asking for years...

About BrickSync... The author decided to vanish and release it as open source. He made a post in the Brick Owl forum.

I made a backup of the code on Github, the one I liked earlier. There I added a couple of updates (new colors) that the community shared. So it is the most up to date out there.

Whoever wants to use it can compile from the files there.

If someone submits a pull request or shares a fix I could accept it. So that whoever uses the new BrickStore can uses it's files with Bricksync.

The most used commands are sort (fills all the fields of the items matching the current inventory) and merge (adds and updates fields).

rgriebl commented 3 years ago

Would it be an option to add an export to ".bsy" files (extension bsy instead of bsx) as a legacy BrickSync export? Or is bricksync hardcoded to expect a bsx extension? I would find it mighty confusing to have normal bsx and legacy bsx exports, while the original BrickStore/Stock don't even need those.

rgriebl commented 3 years ago

I have also (hopefully?) fixed it in bricksync: https://github.com/rgriebl/Bricksync/pull/1 Pick your poison ... I'd personally prefer the fix in bricksync.

paramecie commented 3 years ago

A different extension may be a problem, for ex. to BrickLink or some proprietary (other) tools...

Why not "My important file [LEGACY].bsx" (adding [LEGACY] to the name) ?

ZZJHONS commented 3 years ago

I can't compile it now for my Linux server, will get you back if the fix works as soon as possible. But reviewing it seems it should.

I changed a BrickStock file from .bsx to .bsy and it was processed correctly by BrickSync.

But as paramecie said, it would be better to keep only .bsx as it is what all Lego websites use. Maybe double extension name.old.bsx ?

Will let you know when I can compile and test.

paramecie commented 3 years ago

Ah "myfile.legacy.bsx" would be nice yes!

ZZJHONS commented 3 years ago

Before I get the help to compile and test, could you check if anything else needs to be updated to maintain the currency code of the file, so when opening it again with BrickStore 2021 works correctly?

Line 1076 of the same file for example. fprintf( out, " <Inventory>\n" );

PD: Maybe it is unrelated to the sorting of BSX files and it is to generate something else.

In that case, should BrickStore ask you when opening a file without currency what currency you want to open it with?

If not, I can try to test it tomorrow. Thanks for your time!

ZZJHONS commented 3 years ago

Sort function works perfectly.

This afternoon I will test the merge command, if it works I will accept the Pull Request and this can then be closed.

Thank you!

ZZJHONS commented 3 years ago

Works perfectly fine with this fix.

Thanks for taking the time to investigate and do the Pull Request!

This can now be closed.

BrickSouth commented 3 years ago

@ZZJHONS How do I go about getting the version of Bricksync you have changed with this fix?

ZZJHONS commented 3 years ago

@BrickSouth download all the repository: https://github.com/zzjhons/Bricksync

Delete the folder Releases and compile it.

BrickSouth commented 3 years ago

@ZZJHONS Oh man...looks like you really need to know what you're doing to do the compiling (I obviously do not) Any chance we could get a new release that's already been pre-compiled for us laypeople?

ZZJHONS commented 3 years ago

@BrickSouth please open an issue in the Bricksync repository, and we can discuss there.