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 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.
Anyone else have views about the file part? Or the URL part, for that
matter?
Index: BiblioInfo.cpp
===================================================================
--- BiblioInfo.cpp (revision 33816)
+++ BiblioInfo.cpp (working copy)
@@ -248,6 +248,43 @@
}
+docstring const BibTeXInfo::getFile() const
+{
+ if (is_bibtex_)
+ return operator[]("file");
+ return docstring();
+}
+
+
+docstring const BibTeXInfo::getURL() const
+{
+ if (is_bibtex_) {
+ if (operator[]("url").empty()) {
+ docstring doi = operator[]("doi");
+ if (doi.empty()) {
+ // We could also try falling back to something
like
+ //http://www.google.com.au/search?ie=UTF-8&q=...
+ return docstring();
+ } else {
+ if (prefixIs(doi, from_ascii("http:"))) {
+ return doi;
+ }
+ if (prefixIs(lowercase(doi),
from_ascii("doi:"))) {
+ doi = doi.substr(4);
+ doi = ltrim(doi);
+ }
+
+ return"http://dx.doi.org/"; + doi;
+ }
+ } else {
+ return operator[]("url");
+ }
+ }
+ return docstring();
+}
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.
docstring const BibTeXInfo::getURL() const
{
if (!is_bibtex_)
return docstring();
docstring url = operator[]("url");
if (url.empty()) {
url = operator[]("doi");
if (url.empty())
return url;
}
if (prefixIs(url, from_ascii("http:"))
|| prefixIs(url, from_ascii("https:")))
return url;
docstring prefix = lowercase(url.substr(0,4));
if (prefix == from_ascii("doi:"))
url = ltrim(url.substr(4));
return"http://dx.doi.org/"; + url;
}
// couldn't verify the protocol
return docstring();
}
Index: BiblioInfo.h
===================================================================
--- BiblioInfo.h (revision 33816)
+++ BiblioInfo.h (working copy)
@@ -56,7 +56,9 @@
/// \return the short form of an authorlist
docstring const getAbbreviatedAuthor() const;
///
- docstring const getYear() const;
+ docstring const getFile() const;
+ docstring const getURL() const;
+ docstring const getYear() const;
Something weird here with the whitespace. I think you are using spaces,
whereas LyX uses tabs.
///
docstring const getXRef() const;
/// \return formatted BibTeX data suitable for framing.
@@ -143,7 +145,9 @@
std::vector<docstring> const getEntries() const;
/// \return the short form of an authorlist
docstring const getAbbreviatedAuthor(docstring const& key) const;
- /// \return the year from the bibtex data record for \param key
+ docstring const getFile(docstring const& key) const;
+ docstring const getURL(docstring const& key) const;
Same here.
Index: frontends/qt4/GuiCitation.cpp
===================================================================
--- frontends/qt4/GuiCitation.cpp (revision 33816)
+++ frontends/qt4/GuiCitation.cpp (working copy)
@@ -18,7 +18,9 @@
#include "GuiSelectionManager.h"
#include "qt_helpers.h"
+#include "alert.h"
+
#include "Buffer.h"
#include "BiblioInfo.h"
#include "BufferParams.h"
@@ -56,6 +58,24 @@
static vector<CiteStyle> citeStyles_;
+// 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.
@@ -114,8 +138,14 @@
connect(selectionManager, SIGNAL(okHook()),
this, SLOT(on_okPB_clicked()));
- setFocusProxy(availableLV);
+ if (selected_model_.rowCount()) {
+ setFocusProxy(availableLV);
+ } else {
+ setFocusProxy(selectedLV);
+ //updateControls();
+ }
I'm not sure this is right. The docs say, "setFocusProxy() sets the
widget which will actually get focus when "this widget" gets it." This
would mean that, if you click on the dialog but not on anything in
particular, then 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?
@@ -198,11 +251,13 @@
// two methods, though they should be divisible.
void GuiCitation::updateControls(BiblioInfo const& bi)
{
+ //if (selectionManager->selectedFocused()) {
if (selectionManager->selectedFocused()) {
- if (selectedLV->selectionModel()->selectedIndexes().isEmpty())
+ if (!selectedLV->currentIndex().isValid()) {
updateInfo(bi, availableLV->currentIndex());
- else
+ } else {
updateInfo(bi, selectedLV->currentIndex());
+ }
} else {
if (availableLV->selectionModel()->selectedIndexes().isEmpty())
updateInfo(bi, QModelIndex());
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.
But try not to make cosmetic changes in the patch, since it makes it
harder to see what is actually going on. (Abdel and JMarc had to tell me
that about a gazillion times.)
Index: frontends/qt4/GuiCitation.h
===================================================================
--- frontends/qt4/GuiCitation.h (revision 33816)
+++ frontends/qt4/GuiCitation.h (working copy)
@@ -44,6 +44,8 @@
~GuiCitation();
private Q_SLOTS:
+ void on_urlPB_clicked();
+ void on_filePB_clicked();
void on_okPB_clicked();
void on_cancelPB_clicked();
void on_restorePB_clicked();
@@ -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.
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.
Richard
