This patch got buried when I was distracted by my thesis. Apologies
for the delay.
I address the comments to the previous patch.
Notes:
1) The potential security issue has been addressed by only using LyX's viewers.
2) Richard suggested that I regenerate the filename/url on demand, I
discuss why I don't think this is cleaner: because we cannot update
the other bibliographic information on demand and so generating the
filename on demand increases the likelyhood that a future bug would
cause the filename to become out-of-sync with e.g. the filename
displayed in the tooltip.
3) While I was editing the CitationUi.ui, I cut down on vertical
spacing so that this dialog can display all the buttons on my netbook
with a 1024x600 screen. I think that 1024x600 screens should be
supported though there may be a better way of doing this.
On Tue, Mar 23, 2010 at 10:18 PM, rgheck <rgh...@bobjweil.com> wrote:
>>> 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
OK, the updated patch now fixes used viewURL for URL, and adds a
similar function viewFile, that uses the existing LyX viewers.
> I meant not to use camel case, like "citationFile" but instead to use
> underscores like "citation_file". And yes, you are right, names of private
> members should end in an underscore. I'm not sure why I left that off. I
> think because I was anticipating the next point.
The camel case has been removed.
>>> 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.
>
> What makes this complicated is the code needed to figure out what the
> selected index is. Let me work on this.
It seems to me that GuiCitation::updateInfo() must be called whenever
we change the selection so that e.g. the displayed citation is
updated. Even if it is not called, it is probably best that we open
the file that matches the bibliographic information displayed. For
example, we cannot set the Tooltip on demand so the ToolTip has to be
"cached". May as well "cache" the actual name of the file to be opened
so that even if a bug stops updateInfo being updated we always open
the file listed in the tooltip.
--
John C. McCabe-Dansted
Index: frontends/qt4/GuiCitation.cpp
===================================================================
--- frontends/qt4/GuiCitation.cpp (revision 35745)
+++ frontends/qt4/GuiCitation.cpp (working copy)
@@ -22,12 +22,14 @@
#include "Buffer.h"
#include "BiblioInfo.h"
#include "BufferParams.h"
+#include "Format.h"
#include "FuncRequest.h"
#include "insets/InsetCommand.h"
#include "support/debug.h"
#include "support/docstring.h"
+#include "support/FileName.h"
#include "support/gettext.h"
#include "support/lstrings.h"
@@ -154,6 +156,28 @@
}
+void GuiCitation::on_urlPB_clicked()
+{
+ formats.viewURL(to_filesystem8bit(citation_url_));
+}
+
+
+void GuiCitation::on_filePB_clicked()
+{
+ string filename = to_filesystem8bit(citation_file_);
+ size_t pos = filename.find(":");
+ if (pos != string::npos) {
+ filename = filename.substr(pos + 1);
+ } else {
+ lyxerr << "file entry \"" << filename
+ << "\" does not contain ':' (missing JabRef filetype; "
+ << "but we don't need it anyway)\n";
+ }
+
+ formats.viewFile(FileName(filename));
+}
+
+
void GuiCitation::on_okPB_clicked()
{
applyView();
@@ -363,8 +387,16 @@
return;
}
- QString const keytxt = toqstr(
- bi.getInfo(qstring_to_ucs4(idx.data().toString()), buffer(), true));
+ docstring key = qstring_to_ucs4(idx.data().toString());
+
+ QString const keytxt = toqstr(bi.getInfo(key, buffer(), true));
+ citation_url_ = bi.getURL(key);
+ citation_file_ = bi.getFile(key);
+ urlPB->setEnabled(!citation_url_.empty());
+ urlPB->setToolTip(QString(to_utf8(citation_url_).c_str()));
+ filePB->setEnabled(!citation_file_.empty());
+ filePB->setToolTip(QString(to_utf8(citation_file_).c_str()));
+
infoML->document()->setHtml(keytxt);
}
Index: frontends/qt4/GuiCitation.h
===================================================================
--- frontends/qt4/GuiCitation.h (revision 35745)
+++ 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 citation_file_;
+ /// URL of paper being cited by last clicked entry
+ docstring citation_url_;
};
} // namespace frontend
Index: frontends/qt4/ui/CitationUi.ui
===================================================================
--- frontends/qt4/ui/CitationUi.ui (revision 35745)
+++ frontends/qt4/ui/CitationUi.ui (working copy)
@@ -1,3 +1,4 @@
+<?xml version="1.0" encoding="UTF-8"?>
<ui version="4.0">
<class>CitationUi</class>
<widget class="QDialog" name="CitationUi">
@@ -19,7 +20,7 @@
<item row="0" column="0" colspan="3">
<layout class="QHBoxLayout">
<property name="spacing">
- <number>6</number>
+ <number>1</number>
</property>
<property name="margin">
<number>0</number>
@@ -42,7 +43,7 @@
<property name="sizeHint" stdset="0">
<size>
<width>71</width>
- <height>20</height>
+ <height>02</height>
</size>
</property>
</spacer>
@@ -59,7 +60,7 @@
</item>
</layout>
</item>
- <item row="1" column="0" rowspan="5">
+ <item row="1" column="0" rowspan="7">
<widget class="QListView" name="availableLV">
<property name="editTriggers">
<set>QAbstractItemView::NoEditTriggers</set>
@@ -79,7 +80,7 @@
</property>
</widget>
</item>
- <item row="1" column="2" rowspan="5">
+ <item row="1" column="2" rowspan="7">
<widget class="QListView" name="selectedLV">
<property name="editTriggers">
<set>QAbstractItemView::NoEditTriggers</set>
@@ -107,12 +108,66 @@
<property name="sizeHint" stdset="0">
<size>
<width>77</width>
- <height>71</height>
+ <height>0</height>
</size>
</property>
</spacer>
+ </item>
+ <item row="4" column="1">
+ <widget class="QPushButton" name="filePB">
+ <property name="enabled">
+ <bool>false</bool>
+ </property>
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="Minimum" vsizetype="Fixed">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="toolTip">
+ <string>Open Local Copy of this Cited Work</string>
+ </property>
+ <property name="text">
+ <string>&File</string>
+ </property>
+ <property name="icon">
+ <iconset>
+ <normaloff/>
+ </iconset>
+ </property>
+ <property name="autoDefault">
+ <bool>false</bool>
+ </property>
+ </widget>
</item>
- <item row="4" column="1">
+ <item row="5" column="1">
+ <widget class="QPushButton" name="urlPB">
+ <property name="enabled">
+ <bool>false</bool>
+ </property>
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="Minimum" vsizetype="Fixed">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="toolTip">
+ <string>Open Webpage of Citation</string>
+ </property>
+ <property name="text">
+ <string>&URL</string>
+ </property>
+ <property name="icon">
+ <iconset>
+ <normaloff/>
+ </iconset>
+ </property>
+ <property name="autoDefault">
+ <bool>false</bool>
+ </property>
+ </widget>
+ </item>
+ <item row="6" column="1">
<widget class="QPushButton" name="upPB">
<property name="sizePolicy">
<sizepolicy hsizetype="Minimum" vsizetype="Fixed">
@@ -136,7 +191,7 @@
</property>
</widget>
</item>
- <item row="5" column="1">
+ <item row="7" column="1">
<widget class="QPushButton" name="downPB">
<property name="sizePolicy">
<sizepolicy hsizetype="Minimum" vsizetype="Fixed">
@@ -160,10 +215,10 @@
</property>
</widget>
</item>
- <item row="9" column="0" colspan="3">
+ <item row="11" column="0" colspan="3">
<layout class="QHBoxLayout">
<property name="spacing">
- <number>6</number>
+ <number>1</number>
</property>
<property name="margin">
<number>0</number>
@@ -186,7 +241,7 @@
<property name="sizeHint" stdset="0">
<size>
<width>40</width>
- <height>20</height>
+ <height>02</height>
</size>
</property>
</spacer>
@@ -226,7 +281,7 @@
</item>
</layout>
</item>
- <item row="6" column="0" colspan="3">
+ <item row="8" column="0" colspan="3">
<widget class="QTextEdit" name="infoML">
<property name="acceptDrops">
<bool>false</bool>
@@ -236,7 +291,7 @@
</property>
</widget>
</item>
- <item row="8" column="0" colspan="3">
+ <item row="10" column="0" colspan="3">
<widget class="QGroupBox" name="styleGB">
<property name="title">
<string>Formatting</string>
@@ -299,7 +354,7 @@
<item row="2" column="0" colspan="4">
<layout class="QHBoxLayout">
<property name="spacing">
- <number>6</number>
+ <number>1</number>
</property>
<property name="margin">
<number>0</number>
@@ -312,7 +367,7 @@
<property name="sizeHint" stdset="0">
<size>
<width>21</width>
- <height>20</height>
+ <height>02</height>
</size>
</property>
</spacer>
@@ -342,7 +397,7 @@
</layout>
</widget>
</item>
- <item row="7" column="0" colspan="3">
+ <item row="9" column="0" colspan="3">
<widget class="QGroupBox" name="groupBox">
<property name="title">
<string>Search Citation</string>
@@ -355,7 +410,7 @@
<number>9</number>
</property>
<property name="spacing">
- <number>6</number>
+ <number>01</number>
</property>
<item row="0" column="0">
<widget class="QLabel" name="findKeysLA">
@@ -432,7 +487,7 @@
<property name="sizeHint" stdset="0">
<size>
<width>40</width>
- <height>20</height>
+ <height>02</height>
</size>
</property>
</spacer>
@@ -484,7 +539,7 @@
<property name="sizeHint" stdset="0">
<size>
<width>40</width>
- <height>20</height>
+ <height>02</height>
</size>
</property>
</spacer>
@@ -504,6 +559,8 @@
<zorder>addPB</zorder>
<zorder>selectedLV</zorder>
<zorder>deletePB</zorder>
+ <zorder>filePB</zorder>
+ <zorder>urlPB</zorder>
<zorder>upPB</zorder>
<zorder>downPB</zorder>
<zorder>infoML</zorder>
@@ -515,6 +572,8 @@
<tabstop>selectedLV</tabstop>
<tabstop>addPB</tabstop>
<tabstop>deletePB</tabstop>
+ <tabstop>filePB</tabstop>
+ <tabstop>urlPB</tabstop>
<tabstop>upPB</tabstop>
<tabstop>downPB</tabstop>
<tabstop>findLE</tabstop>
Index: BiblioInfo.h
===================================================================
--- BiblioInfo.h (revision 35745)
+++ BiblioInfo.h (working copy)
@@ -56,6 +56,8 @@
/// \return the short form of an authorlist
docstring const getAbbreviatedAuthor() const;
///
+ docstring const getFile() const;
+ docstring const getURL() const;
docstring const getYear() const;
///
docstring const getXRef() const;
@@ -161,6 +163,8 @@
std::vector<docstring> const getEntries() const;
/// \return the short form of an authorlist
docstring const getAbbreviatedAuthor(docstring const & key) const;
+ docstring const getFile(docstring const & key) const;
+ docstring const getURL(docstring const & key) const;
/// \return the year from the bibtex data record for \param key
/// if \param use_modifier is true, then we will also append any
/// modifier for this entry (e.g., 1998b).
Index: BiblioInfo.cpp
===================================================================
--- BiblioInfo.cpp (revision 35745)
+++ BiblioInfo.cpp (working copy)
@@ -254,6 +254,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();
+}
+
+
+
docstring const BibTeXInfo::getYear() const
{
if (is_bibtex_)
@@ -643,6 +680,28 @@
}
+docstring const BiblioInfo::getFile(docstring const & key) const
+{
+ BiblioInfo::const_iterator it = find(key);
+ if (it == end())
+ return docstring();
+ BibTeXInfo const & data = it->second;
+ docstring file = data.getFile();
+ return file;
+}
+
+
+docstring const BiblioInfo::getURL(docstring const & key) const
+{
+ BiblioInfo::const_iterator it = find(key);
+ if (it == end())
+ return docstring();
+ BibTeXInfo const & data = it->second;
+ docstring url = data.getURL();
+ return url;
+}
+
+
docstring const BiblioInfo::getYear(docstring const & key, bool use_modifier) const
{
BiblioInfo::const_iterator it = find(key);
Index: Format.h
===================================================================
--- Format.h (revision 35745)
+++ Format.h (working copy)
@@ -145,6 +145,8 @@
void setEditor(std::string const & name, std::string const & command);
/// Currently used by hyperlink insets
bool viewURL(std::string const &url);
+ ///
+ bool viewFile(support::FileName const & filename);
/// View the given file. Buffer used for DVI's paper orientation.
bool view(Buffer const & buffer, support::FileName const & filename,
std::string const & format_name) const;
Index: Format.cpp
===================================================================
--- Format.cpp (revision 35745)
+++ Format.cpp (working copy)
@@ -281,6 +281,35 @@
return true;
}
+
+/* viewFile is only used for files LyX doesn't generate itself.
+ * It is also quite similar to viewURL, perhaps they should be merged?
+ */
+
+bool Formats::viewFile(FileName const & filename){
+ Format const * format = getFormat(getFormatFromFile(filename));
+ string command = libScriptSearch(format->viewer());
+
+ if (!contains(command, token_from_format))
+ command += ' ' + token_from_format;
+ command = subst(command, token_from_format, quoteName(filename.toFilesystemEncoding()));
+
+ LYXERR(Debug::FILES, "Executing command: " << command);
+ //buffer.message(_("Executing command: ") + from_utf8(command));
+
+ Systemcall one;
+ int const res = one.startscript(Systemcall::DontWait, command);
+
+ if (res) {
+ Alert::error(_("Cannot view File"),
+ bformat(_("An error occurred whilst running %1$s"),
+ makeDisplayPath(command, 50)));
+ return false;
+ }
+ return true;
+}
+
+
bool Formats::view(Buffer const & buffer, FileName const & filename,
string const & format_name) const
{
