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

Show Favorites as Unread #2766

Merged
merged 15 commits into from
Jan 16, 2020
Merged

Show Favorites as Unread #2766

merged 15 commits into from
Jan 16, 2020

Conversation

Offerel
Copy link
Contributor

@Offerel Offerel commented Jan 13, 2020

Closes #2434

Changes proposed in this pull request:

  • added a setting in "display" to show starred/favorites as unread

How to test the feature manually:

  1. got to settings > display
  2. a new checkbox is available: Show Favorites as "Unread"
  3. enable the checkbox
  4. click on favorites/starred > url parameter state=3 is added to the link
  5. if you click the favorites > read and unread articles in favorites are displayed

Pull request checklist:

  • [ x ] code manually tested

Question:
Anyway, what does "show as unread" mean, with the unread background color or something?

Answer:
Color isn't changed for the articles. All will stay the same. But currently, if you click Favorites/Starred, only unread articles are displayed. In my workflow, when i star/favorite a article, it will be marked as read. Clicking now Favorite/Starred will not show that article, unless i also enable "unread" from the toolbar. This save that step and displays favorites/starred with state=3 as default. If the option is unchecked, the current behavior is used.

Hopefully, this time i get it right :-) It's a bit frickely to do that only in the WebGUI, but i didn't have my PC at hand with git installed

@Frenzie
Copy link
Member

Frenzie commented Jan 13, 2020

The proposed setting looks like this (see at the bottom):
Screenshot_2020-01-13 Display · FreshRSS

I think it should go in reading instead. Both logically and because the "Articles to display" setting is already right there. This is a refinement of that.

I'd name the setting itself something like Always show favorites.

@Frenzie Frenzie added this to the 1.16.0 milestone Jan 13, 2020
@Offerel
Copy link
Contributor Author

Offerel commented Jan 13, 2020

Sure. Feel free to move that to any location you want.

@Frenzie
Copy link
Member

Frenzie commented Jan 13, 2020

Alright, since you don't have access to Git right now I can push a couple of follow-up commits if you don't mind. It's also important to add the translation as a todo to all languages. (There's a PHP helper script for that.)

Frenzie added a commit to Frenzie/FreshRSS that referenced this pull request Jan 13, 2020
While testing <FreshRSS#2766> I noticed that clicking the label activated the wrong dropdown.
@Offerel
Copy link
Contributor Author

Offerel commented Jan 13, 2020

Nice. Many thx for that.

@Frenzie
Copy link
Member

Frenzie commented Jan 13, 2020

Alright, for the moment I made it look like this:
Screenshot_2020-01-13_12-10-50

What I didn't realize before I looked at the code, however, is that is only affects the favorites link. So really it's more like Automatically show all articles in favorites or something along those lines. This may not be an objection to the functionality, but it currently makes the option text incorrect.

marienfressinaud pushed a commit that referenced this pull request Jan 16, 2020
While testing <#2766> I noticed that clicking the label activated the wrong dropdown.
Copy link
Member

@marienfressinaud marienfressinaud left a comment

Choose a reason for hiding this comment

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

You'll need to define the show_fav_unread option in the default configuration file as well.

And I agree with @Frenzie that a better label can be found. Maybe "Show all articles in favorites by default" if not too long?

If you want/need to, I can make the changes myself. Also I think we can close #2535 😉 (sorry for never responding by the way :/)

@@ -31,7 +31,7 @@

<li class="tree-folder category favorites<?= FreshRSS_Context::isCurrentGet('s') ? ' active' : '' ?>">
<div class="tree-folder-title">
<?= _i('bookmark') ?> <a class="title" data-unread="<?= format_number(FreshRSS_Context::$total_starred['unread']) ?>" href="<?= _url('index', $actual_view, 'get', 's') ?>"><?= _t('index.menu.favorites', format_number(FreshRSS_Context::$total_starred['all'])) ?></a>
<?= _i('bookmark') ?> <a class="title" data-unread="<?= format_number(FreshRSS_Context::$total_starred['unread']) ?>" href="<?= _url('index', $actual_view, 'get', 's'); if (FreshRSS_Context::$user_conf->show_fav_unread) echo("&state=3"); ?>"><?= _t('index.menu.favorites', format_number(FreshRSS_Context::$total_starred['all'])) ?></a>
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like how it looks. Suggestion (more lines but easier to understand what's happening):

<?php
    $url_starred = _url('index', $actual_view, 'get', 's');
    if (FreshRSS_Context::$user_conf->show_fav_unread) {
        $url_starred = $url_starred . '&state=3';
    }
?>
<?= _i('bookmark') ?>
<a class="title" data-unread="<?= format_number(FreshRSS_Context::$total_starred['unread']) ?>" href="<?= $url_starred ?>">
    <?= _t('index.menu.favorites', format_number(FreshRSS_Context::$total_starred['all'])) ?>
</a>

@Frenzie
Copy link
Member

Frenzie commented Jan 16, 2020

Show all articles in favorites by default sounds good to me. Please push the changes. 👍

Copy link
Member

@marienfressinaud marienfressinaud left a comment

Choose a reason for hiding this comment

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

Good for me now. I just changed a bit the label from what I first suggested: "Show by default all articles in favorites". Having "by default" at the end sounded a bit like the "favorites" were "by default" and it didn't make any sense.

@@ -97,6 +97,7 @@
),
'reading' => array(
'after_onread' => 'After “mark all as read”,',
'always_show_favorites' => 'Show by default all articles in favorites',
Copy link
Member

Choose a reason for hiding this comment

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

Sorry but no. That's ungrammatical.

Having "by default" at the end sounded a bit like the "favorites" were "by default" and it didn't make any sense.

Not really. "To X (verb) blah blah blah by default" is regular, idiomatic English. The "by default" is about the verb (to X), not about the "blah blah blah." (But I don't think that's any different in French?)

Anyway, you could always phrase it like this. The "articles" business is to remain consistent with the other settings, not by necessity.

Suggested change
'always_show_favorites' => 'Show by default all articles in favorites',
'always_show_favorites' => 'Show all favorites by default',
Suggested change
'always_show_favorites' => 'Show by default all articles in favorites',
'always_show_favorites' => 'Show read and unread favorites by default',

Copy link
Member

Choose a reason for hiding this comment

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

Ha ha, I was certain to make a mistake with this last-minute change :D

Not really. "To X (verb) blah blah blah by default" is regular, idiomatic English. The "by default" is about the verb (to X), not about the "blah blah blah." (But I don't think that's any different in French?)

I think French is a bit more permissive here. First form is right, but can be understood in different ways. That's why I moved the "par défaut" after "Afficher", which is also right in French and avoid the confusion. I changed the English version after that.

I'll pick your first suggestion :)

Copy link
Member

Choose a reason for hiding this comment

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

Edit: finally not, "Show all favorites by default" makes it sound like the first "Always show favorites". "Show all articles in favorites" was pointing the location where the option has an impact. I'll stay with my first suggestion and just adapt for French ;)

Copy link
Member

Choose a reason for hiding this comment

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

You're right, the original suggestion was best. 👍

@marienfressinaud marienfressinaud merged commit 68863fb into FreshRSS:master Jan 16, 2020
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
While testing <FreshRSS#2766> I noticed that clicking the label activated the wrong dropdown.
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>
Co-authored-by: Marien Fressinaud <dev@marienfressinaud.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Optionally display allways read and unread articles in Favorites
3 participants