Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: enable/disable permalinks for RSS #63

Merged
merged 1 commit into from
Feb 7, 2015

Conversation

pikzen
Copy link

@pikzen pikzen commented Nov 21, 2014

The option to see the shortlinks or permalinks has been added to the configuration panel. It is a simple checkbox
This option is disabled by default (meaning that shortlinks are the default)
Updated writeConfig() to save this option
Also fixed a slight typo in config.html.

@e2jk
Copy link

e2jk commented Nov 23, 2014

I have noticed that in function showATOM, the $usepermalinks variable is set as well, but just from the GET variable:

$usepermalinks = isset($_GET['permalinks']);

Should that piece of code also be made aware of the configuration flag? (I haven't tested to run this piece of code myself)

Also, looking at the place where the $usepermalinks variable is used, in both cases the following the isPermaLink guid is set to false:

    <guid isPermaLink="false">

I guess one of the 2 should set it to true?

@pikzen
Copy link
Author

pikzen commented Nov 23, 2014

You're 100% right on the showATOM, the bug report mentioned RSS and I didn't check if it was used elsewhere. Will fix that.

I'm not sure that rainTPL will allow me to dynamically set the RSS/ATOM feed with variables. Will check that too.
EDIT: oh god the RSS is generated by hand. This would probably be a good idea to generate it through RainTPL, both for maintainability and lowering the possible errors when upgrading it.

Thanks for the feedback.

@@ -890,7 +890,8 @@ function showRSS()

// $usepermalink : If true, use permalink instead of final link.
// User just has to add 'permalink' in URL parameters. e.g. http://mysite.com/shaarli/?do=rss&permalinks
$usepermalinks = isset($_GET['permalinks']);
// Also enabled through a config option
$usepermalinks = isset($_GET['permalinks']) || !$GLOBALS['enablersspermalink'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to update the configuration section of the docs (https://github.com/shaarli/Shaarli/wiki#configuration) when this gets merged. The default value should also be explicitely set (default to true?) in the config section of index.php

@nodiscc
Copy link
Member

nodiscc commented Nov 25, 2014

I'm ok to merge this. I'd like some other reviews before going forward.
Thanks

@nodiscc
Copy link
Member

nodiscc commented Dec 9, 2014

@pikzen can you rebase this on top of current master ? As no one seems to disagree, I'll merge it after some testing. It also provides a good example of adding a switch to the config page.

@nodiscc nodiscc mentioned this pull request Dec 17, 2014
@e2jk
Copy link

e2jk commented Dec 17, 2014

@pikzen please rebase, so that we can merge your changes.

@pikzen
Copy link
Author

pikzen commented Dec 18, 2014

The branch has been rebased, along with a slight patch

@nodiscc
Copy link
Member

nodiscc commented Dec 25, 2014

Ok to merge?

@@ -10,7 +10,7 @@
<a href="?do=changetag"><b>Rename/delete tags</b> <span>: Rename or delete a tag in all links</span></a><br><br>
<a href="?do=import"><b>Import</b> <span>: Import Netscape html bookmarks (as exported from Firefox, Chrome, Opera, delicious...)</span></a> <br><br>
<a href="?do=export"><b>Export</b> <span>: Export Netscape html bookmarks (which can be imported in Firefox, Chrome, Opera, delicious...)</span></a><br><br>
<a class="smallbutton" onclick="alert('Drag this link to your bookmarks toolbar, or right-click it and choose Bookmark This Link...');return false;" href="javascript:javascript:(function(){var%20url%20=%20location.href;var%20title%20=%20document.title%20||%20url;window.open('{$pageabsaddr}?post='%20+%20encodeURIComponent(url)+'&amp;title='%20+%20encodeURIComponent(title)+'&amp;description='%20+%20encodeURIComponent(document.getSelection())+'&amp;source=bookmarklet','_blank','menubar=no,height=390,width=600,toolbar=no,scrollbars=no,status=no,dialog=1');})();"><b>✚Shaare link</b></a> <a href="#" style="clear:none;"><span>&#x21D0; Drag this link to your bookmarks toolbar (or right-click it and choose Bookmark This Link....).<br>&nbsp;&nbsp;&nbsp;&nbsp;Then click "✚Shaare link" button in any page you want to share.</span></a><br><br>
<a class="smallbutton" onclick="alert('Drag this link to your bookmarks toolbar, or right-click it and choose Bookmark This Link...');return false;" href="javascript:javascript:(function(){var%20url%20=%20location.href;var%20title%20=%20document.title%20||%20url;window.open('{$pageabsaddr}?post='%20+%20encodeURIComponent(url)+'&amp;title='%20+%20encodeURIComponent(title)+'&amp;description='%20+%20encodeURIComponent(document.getSelection())+'&amp;source=bookmarklet','_blank','menubar=no,height=390,width=600,toolbar=no,scrollbars=no,status=no,dialog=1');})();"><b>✚Shaare link</b></a> <a href="#" style="clear:none;"><span>&#x21D0; Drag this link to your bookmarks toolbar (or right-click it and choose Bookmark This Link....).<br>&nbsp;&nbsp;&nbsp;&nbsp;Then click "✚Shaare link" button in any page you want to share.</span></a><br><br>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Messed up indentation

@e2jk
Copy link

e2jk commented Dec 27, 2014

I haven been able to test, as I'm AFK due to vacation. But looking at the code changes, it makes sense to me. OK to merge (after fixing this minor indentation issue) if someone else has been able to run this branch succesfully.

@pikzen pikzen force-pushed the permaoptions branch 2 times, most recently from 913a0d2 to 8105a24 Compare January 21, 2015 11:02
The option to see the shortlinks or permalinks has been added to the configuration panel. It is a simple checkbox
This option is disabled by default (meaning that shortlinks are the default)
Updated writeConfig() to save this option
Also fixed a slight typo in config.html.

Removed useless CSS & fixed a comment

Enabled permalinks for the ATOM feed and fixed the isPermaLink attribute for the <guid> tag

Reverted to default behavior and clarified its meaning
EnableRssPermalinks is an oddly behaving option: when enabled, it shows a
permalink in the description and a full link in the element title, and
swaps it around when disabled. This clarifies the option for end-users
Also, moved enable_rss_permalinks to $GLOBALS['config'] because it is a
config option.

fix indent
@pikzen
Copy link
Author

pikzen commented Feb 7, 2015

February 7th, I finally managed to fix an indentation issue. 75ish days for basically 4 characters. I've seen more efficient.

Rebased on master, branch is mergeable

@nodiscc nodiscc self-assigned this Feb 7, 2015
nodiscc added a commit that referenced this pull request Feb 7, 2015
New option ENABLE_RSS_PERMALINKS: choose whether the RSS item title link points directly to the link, or to the entry on Shaarli (permalink)
@nodiscc nodiscc merged commit aa69403 into shaarli:master Feb 7, 2015
@nodiscc
Copy link
Member

nodiscc commented Feb 7, 2015

Tested and approved, merging. I'll add this option to the wiki. Maybe the description text on the settings page needs a small clarification, we'll see. Thanks!

@pikzen pikzen deleted the permaoptions branch February 19, 2015 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants