TrilbyWhite / Slider

PDF presentation tool
GNU General Public License v3.0
54 stars 12 forks source link

minor fixups #14

Closed iffsid closed 11 years ago

iffsid commented 11 years ago

ACTION_LAUNCH fix for null l->params gets the rectangle coordinates by default if l->params is null set window class name - for xmonad fixup bug with link mapping

TrilbyWhite commented 11 years ago

Thanks, most of this looks great.

I have two parts I'd like explained though before I merge this. Both are in slider.c

On line 163 you add a block to get the rectangle area. What for? This information is not used for anything. This code only executes if one is selecting an action link with the keyboard. Once the link has been selected, we don't need to know where it was, we just need to execute it's link/code.

On line 229 you are preparing to deal with a null pointer, but there are much easier ways to do this (just setting it to point to a const ""). Further, your code would allocate space for a string, but not free it, then you'd fill that string with numbers (for what purpose?) and try to send those numbers as a parameter of a file. What is the purpose of all of this?

iffsid commented 11 years ago

Both parts are related - A little bit of backstory is required :)

I write my presentations in beamer/LaTeX, and these include videos. (\movie[externalviewer]{\includegraphics{movie_first_frame}}{script_that_plays_movie_with_coords})

Nominally, the way videos would work (if at all on linux machines) is that you'd get mplayer to play the link fullscreen (as indeed you do for video links specifically). However, the nature of my work means that I often have multiple videos per slide and sometimes require that they play simultaneously - something that fullscreening defeats. So, to overcome this issue, what I do is composite a window in root window of slider that is exactly as big as the placeholder for the video (included through \movie[externalviewer]...) for which I need the rectangle, and then run mplayer in that window id, within the coordinate space of the rectangle. So far, I've been using a hacked version of xpdf to do this, but that is one hairy mess - this is much simpler, has more features and is faster!

163: adding a block to get rectangle area is so that if I want to activate an ACTION_LAUNCH from the keyboard, I can get my external movie playing script the coordinates required. This was being done for mouse input, but was missing for the keyboard input; that's why I put it in.

229: you're right that I could just set it to "". In fact the right thing to do is if l->params was NULL, then set it to const "" and then append the rectangle coordinates instead of what I did - because if there are parameters, then I lose the use of my coordinates; further, if you didn't need the coordinates, then you just wouldn't use the last 4 options in your script. If you think that's the better option, then let me know and I'll make the change in my next pull request.

where I'm going with this: I'd like to have the option to play the action_launch links automatically as I enter that slide and also kill the movie (played through the external player) if I so choose. I currently have the former mostly done (introduced "auto" as an alternative to "keys" and "mouse"), and I'm looking into the latter. Let me know what you think. :)

TrilbyWhite commented 11 years ago

Ah, that makes perfect sense. I hadn't considered any such uses, but it's a great idea - I'd also be happy to include the auto option.

The action links - in general - are relatively new and quite ugly, script/executable links are virtually untouched and completely untested. Partially because I don't use them, but also because of the caution needed with them (as stated in my comment by the conditional compile test for them). My only personal goal for links was to allow simple media (a sound clip or video file) to play. But if so little code is needed to allow for such creative use, I'm all for it.

Thanks for the code and the explanation.