Skip to content

Commit

Permalink
Rework how we open urls that have a #
Browse files Browse the repository at this point in the history
Previously if it was a remote url that had # and a . after the # we
assumed the url had no fragment and everything was filename.

We don't do that anymore, what we do now is try to open the url as
parsed, i.e. before the # is the filename after is the fragment, and if
that fails we try to open everything as filename and nothing as
fragment.

Unfortunately given how kpart internals handle opening local vs remote
urls we need to do this in two places.

Also we have to remove the test that checked that the url was mangled at
the shell level because we don't do that anymore. Unfortunately can't
add a test for the new codepage since it would involve starting an http
server ^_^

Filenames:
  source2e.pdf
  foo#bar.pdf

What works:
 * okular http://localhost/source2e.pdf#subsection.68.3
 * okular file:///srv/http/source2e.pdf#subsection.68.3
 * okular source2e.pdf#subsection.68.3 (in the /srv/http folder)
 * okular source2e.pdf#2
 * okular http://localhost/foo#bar.pdf
 * okular file:///srv/http/foo#bar.pdf
 * okular foo#bar.pdf (in the /srv/http folder)

What doesn't work:
 * okular http://localhost/foo#bar.pdf#2

I think it's a fair limitation that if you want to open a file that contains # in the name and also use a # page marker you need to use the encoded url like okular http://localhost/foo%23bar.pdf#2
after all things like firefox will totally fail opening http://localhost/foo#bar.pdf and will just work if you give the encoded url

BUGS: 426976
  • Loading branch information
tsdgeos committed Nov 26, 2020
1 parent 023976c commit 239827b
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 18 deletions.
6 changes: 0 additions & 6 deletions autotests/shelltest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,6 @@ void ShellTest::testUrlArgs_data()

// non-local files
QTest::newRow("http://kde.org/foo.pdf") << "http://kde.org/foo.pdf" << true << QUrl(QStringLiteral("http://kde.org/foo.pdf"));
// make sure we don't have a fragment
QUrl hashInName(QStringLiteral("http://kde.org"));
QVERIFY(hashInName.path().isEmpty());
hashInName.setPath(QStringLiteral("/foo#bar.pdf"));
QVERIFY(hashInName.fragment().isEmpty());
QTest::newRow("http://kde.org/foo#bar.pdf") << "http://kde.org/foo#bar.pdf" << true << hashInName;
QUrl withAnchor(QStringLiteral("http://kde.org/foo.pdf"));
withAnchor.setFragment(QStringLiteral("anchor"));
QTest::newRow("http://kde.org/foo.pdf#anchor") << "http://kde.org/foo.pdf#anchor" << true << withAnchor;
Expand Down
27 changes: 22 additions & 5 deletions part/part.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,9 @@ void Part::loadCancelled(const QString &reason)
// so we don't want to show an ugly messagebox just because the document is
// taking more than usual to be recreated
if (m_viewportDirty.pageNumber == -1) {
if (!reason.isEmpty()) {
if (m_urlWithFragment.isValid() && !m_urlWithFragment.isLocalFile()) {
tryOpeningUrlWithFragmentAsName();
} else if (!reason.isEmpty()) {
KMessageBox::error(widget(), i18n("Could not open %1. Reason: %2", url().toDisplayString(), reason));
}
}
Expand Down Expand Up @@ -1685,6 +1687,7 @@ bool Part::openUrl(const QUrl &_url, bool swapInsteadOfOpening)

QUrl url(_url);
if (url.hasFragment()) {
m_urlWithFragment = _url;
const QString dest = url.fragment(QUrl::FullyDecoded);
bool ok = true;
int page = dest.toInt(&ok);
Expand All @@ -1709,6 +1712,8 @@ bool Part::openUrl(const QUrl &_url, bool swapInsteadOfOpening)
m_document->setNextDocumentDestination(dest);
}
url.setFragment(QString());
} else {
m_urlWithFragment.clear();
}

// this calls in sequence the 'closeUrl' and 'openFile' methods
Expand All @@ -1719,15 +1724,27 @@ bool Part::openUrl(const QUrl &_url, bool swapInsteadOfOpening)

setWindowTitleFromDocument();
} else {
resetStartArguments();
/* TRANSLATORS: Adding the reason (%2) why the opening failed (if any). */
QString errorMessage = i18n("Could not open %1. %2", url.toDisplayString(), QStringLiteral("\n%1").arg(m_document->openError()));
KMessageBox::error(widget(), errorMessage);
if (m_urlWithFragment.isValid() && m_urlWithFragment.isLocalFile()) {
openOk = tryOpeningUrlWithFragmentAsName();
} else {
resetStartArguments();
/* TRANSLATORS: Adding the reason (%2) why the opening failed (if any). */
QString errorMessage = i18n("Could not open %1. %2", url.toDisplayString(), QStringLiteral("\n%1").arg(m_document->openError()));
KMessageBox::error(widget(), errorMessage);
}
}

return openOk;
}

bool Part::tryOpeningUrlWithFragmentAsName()
{
QUrl url = m_urlWithFragment;
url.setPath(url.path() + QLatin1Char('#') + url.fragment());
url.setFragment(QString());
return openUrl(url);
}

bool Part::queryClose()
{
if (!isReadWrite() || !isModified())
Expand Down
6 changes: 6 additions & 0 deletions part/part.h
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,8 @@ public Q_SLOTS:
void slotShareActionFinished(const QJsonObject &output, int error, const QString &message);
#endif

bool tryOpeningUrlWithFragmentAsName();

static int numberOfParts;

QTemporaryFile *m_tempfile;
Expand Down Expand Up @@ -421,6 +423,10 @@ public Q_SLOTS:
// String to search in document startup
QString m_textToFindOnOpen;

// Set when opening an url that had fragment so that if it fails opening we try adding the fragment to the filename
// if we're opening http://localhost/foo#bar.pdf and the filename contains an # we can open it after trying to open foo fails
QUrl m_urlWithFragment;

private Q_SLOTS:
void slotAnnotationPreferences();
void slotHandleActivatedSourceReference(const QString &absFileName, int line, int col, bool *handled);
Expand Down
7 changes: 0 additions & 7 deletions shell/shellutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,6 @@ QUrl urlFromArg(const QString &_arg, FileExistFunc exist_func, const QString &pa
url.setPath(path.left(hashIndex));
url.setFragment(path.mid(hashIndex + 1));
}
} else if (!url.fragment().isEmpty()) {
// make sure something like http://example.org/foo#bar.pdf is treated as a path name
// but something like http://example.org/foo.pdf#bar is foo.pdf plus an anchor "bar"
if (url.fragment().contains(QLatin1Char('.'))) {
url.setPath(url.path() + QLatin1Char('#') + url.fragment());
url.setFragment(QString());
}
}
if (!pageArg.isEmpty()) {
url.setFragment(pageArg);
Expand Down

0 comments on commit 239827b

Please sign in to comment.