On Tue, Mar 23, 2010 at 4:25 AM, rgheck <rgh...@bobjweil.com> wrote:
> On 03/22/2010 03:14 PM, John McCabe-Dansted wrote:
>
> The attached patch resolved the following tickets:
> #6486 Citation not selected by default.
> #6487 Add "file" and "url" links to Citation Dialog.
...
> Please try to attach the patch in such a way that it'll show up in the email
> and as an attachment. This makes commenting on the patch much easier.
I could paste the patch into the email, but I don't think that is what
you mean. Could you give an example of an email client that can do
this? (I don't think the gmail webclient can).
> I haven't tested this or even looked at the UI. Just some general comments.
> But generally it looks good to me.
>
> That said, I'm a bit unsure about the file part. There are security risks
> there. True, people usually put pdfs there, not shell scripts, but you can
> see the worry. By the same token, we might want to check the URL and make
> sure it has an appropriate prefix before launching. You can put anything in
> the url field, too.
Thanks. I will think about this. Another way might be to extend the
existing viewer launching code so that it can pick up the filetype
from the extension. (I note that prefixing all files with file://
doesn't help on Windows), I can't seem to find any guides on safe ways
to launch a viewer of untrusted attachments.
> That can all be simplified using early returns, and give us a lot less
> nesting, too. I've added some error checking, too, as suggested above.
Thanks.
> +// Tells OS to open a File or URL
> +// Formats::view does not seem appropriate as it wants a Buffer
> +static void system_viewer (string url) {
> + string open_cmd;
> + #ifdef _WIN32
> + open_cmd = "start";
> + #elif defined(USE_MACOSX_PACKAGING)
> + open_cmd = "open";
> + #else
> + open_cmd = "xdg-open";
> + #endif
> + string cmd = open_cmd + " \"" + url + "\" &";
> + int result = system(cmd.c_str());
> + if (result) {
> + Alert::warning(_("Could not open file"), bformat(_("Command
> \n$1%s\n
> Exited with Error#$2%d"), from_local8bit(cmd), result));
> + }
> +}
> +
>
> What about Systemcall::startscript (which needn't start a *script*)? This is
> in support/Systemcall.{h,cpp}. That is generally where you will find useful
> things like this.
I could use this but it doesn't seem to have any support for opening a
viewer, so it wouldn't be any simpler than the above code. Also if
"DontWait" is set it uses "start /min" but presumably we don't want
the view to be minimized. I guess this might be a good place to put
code for launching a viewer, if we don't create a buffer-free variant
of the current viewer code.
...
> focus goes to the thing defined. (Yes, I know it was that way before.) I'd
> try something like:
> availableLV->setFocus( Qt::ActiveWindowFocusReason)
> and see if it works. I assume the dialog gets focus when we create it, yes?
OK.
...
> This was OK as it was. The LyX code often omits these brackets. It's a
> matter of taste whether one puts them in or not. I find it can clutter the
> code.
I added extra code, removed the extra code and forgot to remove the
{}'s. I will fix this.
...
> @@ -166,6 +168,10 @@
> QStringList cited_keys_;
> ///
> InsetCommandParams params_;
> + /// File containing paper being cited by last clicked entry
> + docstring citationFile_;
> + /// URL of paper being cited by last clicked entry
> + docstring citationURL_;
> };
>
> Local variables should be like: citation_file, citation_url.
I am not sure what you mean, e.g. params_ is also "local" to an
instance of a object and it ends in _; all of the other per object
variables in LyX seem to end in _.
> A more general question is whether you really need or want these variables.
> I think we can just re-calculate if one of the buttons gets pushed. This
> will not take enough time to be noticeable. Caching the values costs memory,
> and it means we have to be careful always to keep it in sync with the
> dialog, which invites bugs.
Well, the issue isn't so much time as deciding which citation we want
to open. To know this we would have to decide which LV has focus (what
if neither has focus?) and then pick the selected item of the LV. This
seems more complex and bug prone than updating citationURL_ and
citationFile_ in the same place we update keytxt. Keeping two extra
filenames in memory probably isn't any more noticeable than the time
needed to recalculate them.
Alternatively we could cache the BiblioInfo from which we calculate
citationURL_ and citationFile_, but this seems at least as complex as
caching citationURL_ and citationFile_.
--
John C. McCabe-Dansted
