|
Description
Jakub Alba
2016-07-24 21:14:32 UTC
Created attachment 125298 [details]
setprops.c
Created attachment 125299 [details] [review] [PATCH 1/2] introduced hex string as a new Object type and used it for file identifier Created attachment 125300 [details] [review] [PATCH 2/2] init new file identifier object as a hex string Created attachment 125301 [details] [review] [PATCH 1/2] introduced hex string as a new Object type and used it for file identifier Oops, forgot about the copyright info in the previous 1/2 patch. Created attachment 125343 [details] [review] [PATCH 2/2] treat file identifier as a hex string, not a basic string Fixed other parts of poppler to use hex strings when dealing with file identifiers. Please add the enum to the end not in the middle Thomas, this looks reasonable to me but since you wrote part of the "pdf writing" code i'd appreciate a quick review from your side if you have time. Created attachment 125439 [details] [review] [PATCH 1/2] introduced hex string as a new Object type and used it for file identifier (In reply to Albert Astals Cid from comment #6) > Please add the enum to the end not in the middle Done. Comment on attachment 125439 [details] [review] [PATCH 1/2] introduced hex string as a new Object type and used it for file identifier objTypeNames diff is wrong now i guess? Created attachment 125440 [details] [review] [PATCH 1/2] introduced hex string as a new Object type and used it for file identifier Yes. Sorry for that. Fixed. (In reply to Albert Astals Cid from comment #7) > Thomas, this looks reasonable to me but since you wrote part of the "pdf > writing" code i'd appreciate a quick review from your side if you have time. The patches are reasonable and look good for me After having a look at the first patch, i was wondering if we really need the "hex" variable in the union, can't we just use string? Created attachment 125498 [details] [review] [PATCH 1/2] introduced hex string as a new Object type and used it for file identifier (In reply to Albert Astals Cid from comment #12) > After having a look at the first patch, i was wondering if we really need > the "hex" variable in the union, can't we just use string? I added it, because I'd had some problems when writing this code, but now, when I checked once again, it seems that using string is perfectly OK. Created attachment 127476 [details] [review] [PATCH 1/2] introduced hex string as a new Object type and used it for file identifier Created attachment 127477 [details] [review] [PATCH 2/2] treat file identifier as a hex string, not a basic string I've updated my name and e-mail in the copyright info and commit author info. (In reply to Jakub Alba from comment #13) > Created attachment 125498 [details] [review] [review] > [PATCH 1/2] introduced hex string as a new Object type and used it for file > identifier > > (In reply to Albert Astals Cid from comment #12) > > After having a look at the first patch, i was wondering if we really need > > the "hex" variable in the union, can't we just use string? > > I added it, because I'd had some problems when writing this code, but now, > when I checked once again, it seems that using string is perfectly OK. So can you please update the patches to use string? (In reply to Albert Astals Cid from comment #17) > (In reply to Jakub Alba from comment #13) > > Created attachment 125498 [details] [review] [review] [review] > > [PATCH 1/2] introduced hex string as a new Object type and used it for file > > identifier > > > > (In reply to Albert Astals Cid from comment #12) > > > After having a look at the first patch, i was wondering if we really need > > > the "hex" variable in the union, can't we just use string? > > > > I added it, because I'd had some problems when writing this code, but now, > > when I checked once again, it seems that using string is perfectly OK. > > So can you please update the patches to use string? They do use string (the variable in the union). They just still introduce a new enum element just to carry the information if the string is a hex string. Or am I missing something? You're right sorry, i forgot what this was about :D |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.