search

Kalpanika / x3f

Tools for manipulating X3F files from Sigma cameras
88 stars 28 forks source link

Better user experience #97

Closed rolkar closed 7 years ago

rolkar commented 7 years ago

This pull request contains three things

  1. unlink of the tmp file before creation of tmp file. This avoids getting bogus error reports from users, saying that there is some error in file writing, when it really is a rogue tmp file from previous crasch.

  2. A bit more info when trying to extract stuff and it fails.

  3. A test for non known RAW file format. So, you know that is the problem. Quite important when Sigmna change stuff or add new cameras.

mmroden commented 7 years ago

I'll put out a new copy of the wrapper once we have this going. I'm also looking to add in the new sample files into the test project, so we can have some real tests going there.

rolkar commented 7 years ago

I have a huge collection of sample files for most cameras on my computer.

BTW - there will be a fix for sd Quattro and one for sd Quattro H. The first one might take a week, or two. The second one is unknown amount of work.

mmroden commented 7 years ago

I get that you don't want to do a full rewrite, but this is just raising a red flag for me. Before, the question was whether the function was returning ok, and now it looks like the question is whether the assignment of ret is ok.

On May 1, 2017, 8:50 AM -0700, Roland Karlsson notifications@github.com, wrote:

@rolkar commented on this pull request.

In src/x3f_extract.c (https://github.com/Kalpanika/x3f/pull/97#discussion_r114144123):

@@ -309,38 +310,57 @@ int main(int argc, char *argv[]) } if (extract_jpg) { - if (X3F_OK != x3f_load_data(x3f, x3f_get_thumb_jpeg(x3f))) { - x3f_printf(ERR, "Could not load JPEG thumbnail from %s\n", infile); + if (X3F_OK != (ret = x3f_load_data(x3f, x3f_get_thumb_jpeg(x3f)))) {

This is a quite common way to do it. And ... it was essentially the same in the old code. The only difference is that it extracts ret, to be used in the error printout.

I do not feel like doing some refactoring of the code for style's sake. We have bigger problems with this code than that.

This is a preparation for solving the sd Quattro firmware 1.04 bug and also preparing for supporting the the sd Quattro H. To get better error printouts so we see what happens.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/Kalpanika/x3f/pull/97#discussion_r114144123), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAHGG1XdYqUka7nrb4SL96VPIiQXG3YZks5r1f8_gaJpZM4NNIkl).

rolkar commented 7 years ago

if (X3F_OK == (ret = foo())) ploing();

Does not test if the assignment is X3F_OK. It test if the return value from foo() is X3F_OK.

In C e.g. "a = (b = foo())" sets both a an b to the return value of foo(). The return value from an assignment is equal to the right hand side value. There is no special return value from an assignment.

rolkar commented 7 years ago

The alternative would be to write

ret = x3f_load_data(x3f, x3f_get_thumb_jpeg(x3f)); if (X3F_OK != ret) { ...

mmroden commented 7 years ago

Right :)

Setting variables in an if statement are flat illegal in python, cuz it has been the cause of huge pain.

Like I said, tho, this is me not being that swift with the C, so if we're all good and you guys read this like the morning paper, then go ahead and merge.

On Mon, May 1, 2017 at 9:04 AM, Roland Karlsson notifications@github.com wrote:

The alternative would be to write

ret = x3f_load_data(x3f, x3f_get_thumb_jpeg(x3f)); if (X3F_OK != ret) { ...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Kalpanika/x3f/pull/97#issuecomment-298362363, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHGG2tqJtgCLPxR5sLLTqhTU-W9y-xXks5r1gKNgaJpZM4NNIkl .

rolkar commented 7 years ago

:)

mmroden commented 7 years ago

Please lemme know when there's another exe, so I can update the wrapper. Or is the goal to release after fixing the new quattro files?

On Mon, May 1, 2017 at 10:20 AM, Roland Karlsson notifications@github.com wrote:

Merged #97 https://github.com/Kalpanika/x3f/pull/97.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Kalpanika/x3f/pull/97#event-1064225005, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHGG3Qg-dJZ1VsfHBAR06TUJg4U5NIUks5r1hRVgaJpZM4NNIkl .

rolkar commented 7 years ago

Yes, the goal is to release after fixing sd Quattro H and the new firmware for sd Quattro.

I made a fast test with sd Quattro H, in the fork:branch rolkar:sdqh.

It works! But .... the dots are back, of course, as the position for the sdQ H is hard coded. So, now it is not an unknown time any more.

Both shall be possible to fix ASAP.

/Roland

On 2017-05-01 19:55, mmroden wrote:

Please lemme know when there's another exe, so I can update the wrapper. Or is the goal to release after fixing the new quattro files?

On Mon, May 1, 2017 at 10:20 AM, Roland Karlsson notifications@github.com wrote:

Merged #97 https://github.com/Kalpanika/x3f/pull/97.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Kalpanika/x3f/pull/97#event-1064225005, or mute the thread

https://github.com/notifications/unsubscribe-auth/AAHGG3Qg-dJZ1VsfHBAR06TUJg4U5NIUks5r1hRVgaJpZM4NNIkl .

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/Kalpanika/x3f/pull/97#issuecomment-298387330, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVzmKsBoE1D9VaLqYzK-ZOTvsdI6ZT0ks5r1hyugaJpZM4NNIkl.

Detta e-postmeddelande har sökts igenom efter virus med antivirusprogram från Avast. https://www.avast.com/antivirus

mmroden commented 7 years ago

Oh, awesome. Sounds good.

I need to add the H files into the test images project, I'll try to get that done today.

On Mon, May 1, 2017 at 11:02 AM, Roland Karlsson notifications@github.com wrote:

Yes, the goal is to release after fixing sd Quattro H and the new firmware for sd Quattro.

I made a fast test with sd Quattro H, in the fork:branch rolkar:sdqh.

It works! But .... the dots are back, of course, as the position for the sdQ H is hard coded. So, now it is not an unknown time any more.

Both shall be possible to fix ASAP.

/Roland

On 2017-05-01 19:55, mmroden wrote:

Please lemme know when there's another exe, so I can update the wrapper. Or is the goal to release after fixing the new quattro files?

On Mon, May 1, 2017 at 10:20 AM, Roland Karlsson notifications@github.com wrote:

Merged #97 https://github.com/Kalpanika/x3f/pull/97.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Kalpanika/x3f/pull/97#event-1064225005, or mute the thread

https://github.com/notifications/unsubscribe-auth/AAHGG3Qg- dJZ1VsfHBAR06TUJg4U5NIUks5r1hRVgaJpZM4NNIkl .

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/Kalpanika/x3f/pull/97#issuecomment-298387330, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVzmKsBoE1D9VaLqYzK- ZOTvsdI6ZT0ks5r1hyugaJpZM4NNIkl.

Detta e-postmeddelande har sökts igenom efter virus med antivirusprogram från Avast. https://www.avast.com/antivirus

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Kalpanika/x3f/pull/97#issuecomment-298389033, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHGGyx9_dSwK9KT6AvLEB4wGXh3UrvEks5r1h5JgaJpZM4NNIkl .