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

Keep private links number … private. #2109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thican
Copy link

@thican thican commented Nov 16, 2024

I guess the title is self explanatory.

IMHO, this value should not be seen for public, unless this is an open instance and therefore, no logged is kind of already logged in.

@thican thican changed the title Keeps private links number … private. Keep private links number … private. Nov 16, 2024
@thican
Copy link
Author

thican commented Nov 16, 2024

I think it should be integrated for milestone 0.14.0, as raised in discussion #2105.

nodiscc
nodiscc previously approved these changes Nov 17, 2024
Copy link
Member

@nodiscc nodiscc left a comment

Choose a reason for hiding this comment

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

It is still easy to determine the number of private shaares by substracting the number of visible shaares, from the total shaares counter

        $this->assignView('linkcount', $this->container->bookmarkService->count(BookmarkFilter::$ALL));
        $this->assignView('privateLinkcount', $this->container->bookmarkService->count(BookmarkFilter::$PRIVATE));

A better approach would be displaying only the count of public shaares.

Anyway this doesn't hurt, so approving

@nodiscc nodiscc added this to the 0.14.0 milestone Nov 17, 2024
@thican
Copy link
Author

thican commented Nov 17, 2024

It is still easy to determine the number of private shaares by substracting the number of visible shaares, from the total shaares counter

        $this->assignView('linkcount', $this->container->bookmarkService->count(BookmarkFilter::$ALL));
        $this->assignView('privateLinkcount', $this->container->bookmarkService->count(BookmarkFilter::$PRIVATE));

A better approach would be displaying only the count of public shaares.

Oh you’re totally right, I didn’t notice this issue.
Yes this was my objective, only displaying those public ones.

Anyway this doesn't hurt, so approving

Please don’t merge until I (or anybody) fix this for only those public shaares number.

@thican thican force-pushed the keep_private_links_number_private branch from 987ece6 to 14aecaa Compare November 18, 2024 01:07
@thican
Copy link
Author

thican commented Nov 18, 2024

@nodiscc Done.

I allowed myself to display the number of links, whether total, private & public, even if 0, instead of masking the information.

@thican thican force-pushed the keep_private_links_number_private branch 3 times, most recently from 530e58e to 58fe58c Compare November 18, 2024 20:53
@thican
Copy link
Author

thican commented Nov 18, 2024

I noticed there is translation feature, no idea if it was required and how to actually update, so my modifications are flawed as I copied them (without the total word that I added).

@bttrx
Copy link

bttrx commented Dec 5, 2024

I had the same idea in September and patched the default theme to a custom one:

--- p/tpl/default/linklist.html	Sat Mar 18 13:45:18 2023
+++ p/tpl/robert/linklist.html	Sun Sep 29 19:37:39 2024
@@ -9,7 +9,7 @@
 <div class="linkcount pure-u-0 pure-u-lg-visible">
   {if="!empty($linkcount)"}
   <span class="strong">{$linkcount}</span> {function="t('shaare', 'shaares', $linkcount)"}
-  {if="$privateLinkcount>0"}
+  {if="$is_logged_in && $privateLinkcount>0"}
   <br><span class="strong">{$privateLinkcount}</span> {function="t('private link', 'private links', $privateLinkcount)"}
   {/if}
   {/if}
@@ -58,7 +58,7 @@
       <div class="linkcount pure-u-lg-0 center">
         {if="!empty($linkcount)"}
         <span class="strong">{$linkcount}</span> {function="t('shaare', 'shaares', $linkcount)"}
-        {if="$privateLinkcount>0"}
+        {if="$is_logged_in && $privateLinkcount>0"}
         &middot; <span class="strong">{$privateLinkcount}</span> {function="t('private link', 'private links', $privateLinkcount)"}
         {/if}
         {/if}

@nodiscc nodiscc self-requested a review December 5, 2024 21:15
@nodiscc nodiscc modified the milestones: 0.14.0, 0.15.0 Dec 8, 2024
@thican thican force-pushed the keep_private_links_number_private branch from 58fe58c to e27156f Compare December 8, 2024 15:59
@thican
Copy link
Author

thican commented Dec 8, 2024

I had the same idea in September and patched the default theme to a custom one:
../..

In your example, people can still determine the number of private links because the main number contains both public and private links from the number of pages of links.

@bttrx
Copy link

bttrx commented Dec 10, 2024

In your example, people can still determine the number of private links because the main number contains both public and private links from the number of pages of links.

Ah, now I see, what you mean.

I just checked a fresh installation. Shaarli shows a number of 3 shaares at the top right, but only 1 shaare is shown in the list. So, there are 2 private links.

Copy link
Member

@nodiscc nodiscc left a comment

Choose a reason for hiding this comment

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

I would revert the translation and wording change (shaare(s) -> total share(s)) as ti still works and makes sense without this change, and will make this easier to review.

I don't remember how the translation system works right now.

@nodiscc nodiscc dismissed their stale review December 13, 2024 10:19

old review

@thican thican force-pushed the keep_private_links_number_private branch from e27156f to fe4504a Compare December 13, 2024 23:52
@thican
Copy link
Author

thican commented Dec 13, 2024

I would revert the translation and wording change (shaare(s) -> total share(s)) as ti still works and makes sense without this change, and will make this easier to review.

Done.

@nodiscc nodiscc self-requested a review December 14, 2024 11:03
@thican thican force-pushed the keep_private_links_number_private branch from fe4504a to e461548 Compare December 16, 2024 23:45
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