This commit enhances the Capture Screenshot keyword to support
internal running counter also when filename is not None. The place
of the counter is indicated with %d character.

Also the absolute path of the screenshot filename is always returned
by the keyword.

Also made small drive by refactoring to follow pep8 better.

I initially proposed %d as a placeholder for the index. It's an interesting idea to possible support also other formatting options, but after thinking a bit I don't think that would be very useful. Such formatting would work well if there would be other automatically generated information, but I doubt the would be. Formatting wouldn't be too useful in hypothetical cases like

Capture Page Screenshot    foo-{bar}-{index}.png    bar=zap
Capture Page Screenshot    foo-{bar}-{index}.png    bar=${var}

because you could just use

Capture Page Screenshot    foo-zap-{index}.png
Capture Page Screenshot    foo-${var}-{index}.png

Because I think other formatting isn't likely very useful, I don't think we need to take that into account. That means we could easily use something more explicit than %d to specify an index is needed. We could, for example, allow <index> or :index: and just use .replace('<index>', index) in the code.

Also, @aaltat mentioned that str.format cannot be used because Python 2.5 needs to be supported. I think Selenium has dropped 2.5 support years ago so that shouldn't be an issue. Is Python 2.5 support promised somewhere?

It is not promised directly. But we do claim to support quite old versions of Robot Framework (2.6 is the minimum version) and older versions do support Python 2.5. How realistic this is, ido not know.

But if we announced that we only support 2.6 onwards, it would allow to use str.format. I still keep the formatting option open because it would be really nice to get example 00001, 00002... and not 1, 2... Or perhaps adding epoch would be nice too.

As I already wrote, Selenium itself hasn't supported Py2.5 for years. Do you think someone would be using new S2L and downgrade Se separately to an ancient version?

Supporting formatting like leading zeros is a very good point. Supporting {index} and using .format() sounds like a good idea.

Ah my mistake, in INSTALL.rst we say that 2.6 is the minimum version. Because of the trouble caused by %d, I will change the implementation so direction which was suggested by @pekkaklarck. The str.format feels also quite temping to use, because it opens quite nice formatting possibilities. Perhaps it would be best to do something like this: python -c "print 'file-name-{index}.png'.format(index=1)"

Refactored the pull request to use str.format

I just read through it and everything looks good to me. You've got my 👍 to merge whenever you feel this is ready.

If @pekkaklarck is also happy, then I am also ready for merge

Squashed all to commits to one, because the middle commits where not meaningful alone.

Hmm some other test did fail, can I somehow push Travis to rerun the tests?

I added few more line notes. Please take a look at them @aaltat and fix ones that you see worth fixing. Would you @zephraph like take a final look at this afterwards and merge?

@aaltat, have you ran this locally to make sure that the screenshots really do show up?

I did manualy verify that screen captures are visible in the log.

Found two more small typos. Otherwise, it looks good to me. Fix those and I'll merge it in.

Thanks for your patience!

Fixed based on @zephraph comments.

I think, this is normal iterative development process. I did have an idea, or actually problem to solve. Made a first attempt to fix it and honesty it was quite horrible attempt. Then there was new idea from the review process, which was better, but caused lot of problems/corner cases. And on last attempt, we did nail it down quite well.

Also I am used to review process, it is nice way to share the information between people. It also yields to better results (and this is fine example of one), like better functionality, cleaner code and so one.