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

opt:favorite icon adjustment,allow more situation #2178

Closed
wants to merge 8 commits into from

Conversation

xiaoyifang
Copy link
Owner

@xiaoyifang xiaoyifang commented Feb 19, 2025

icon
image word contain in both current associated folder and existed in other folders as well
image not existed in current associated folder but existed in other folders
image not in favorite
image in current associate folder

Comment on lines 42 to 43
//key hold the headword, value holds the folders.
QMap< QString, QSet< QString > > folderFavoritesMap;
Copy link
Collaborator

@shenlebantongying shenlebantongying Feb 19, 2025

Choose a reason for hiding this comment

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

Maybe name this thing better?

https://www.google.com/search?q=map+naming+convention

  • wordToFavoritesFolders(Map),wordToFavFolders(Map)
  • wordFavoritesFoldersMap,wordFavFoldersMap
  • (get)FavoritesFoldersByWord,(get)FavFoldersByWord

Copy link
Owner Author

Choose a reason for hiding this comment

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

ok

@xiaoyifang xiaoyifang marked this pull request as draft February 19, 2025 13:54
@xiaoyifang xiaoyifang force-pushed the opt/favorite-refactor branch from 42161e2 to b3fda44 Compare February 21, 2025 13:29
Comment on lines +163 to +166
QIcon emptyIcon = QIcon( ":/icons/star.svg" );
QIcon fullIcon = QIcon( ":/icons/star_blue.svg" );
QIcon emptyFullIcon = QIcon( ":/icons/star_other.svg" );
QIcon fullFullIcon = QIcon( ":/icons/star_blue_other.svg" );
Copy link
Collaborator

@shenlebantongying shenlebantongying Feb 21, 2025

Choose a reason for hiding this comment

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

I doubt if most of the users could infer the meaning of 4 states of this icons 😕 .

Cannot we have it simpler?

  • 🌟 "blue" icon -> in favorites regardless of group/folder choice
  • ⚪ empty -> not in favorites regardless of group/folder choice

I guess the original intention was to let a user putting word directly into a folder.

Currently, without associating a group with a folder, the word will end up in fav's top most level.

How about we just associate with the current active selected folder instead.

This fix the original issue of "not be able to put word to folder directly" and also reduces complexity by decoupling group⇄folder association.

For example, when clicking the star icon, the word will end up in the selected favoriate folder.

image


If I design this from scratch, I won't do the original GD's group->folder association 😄.

But we can keep this feature

click star icon:

if (have group->folder association)
  put word in the group's folder choice
else
  put word in current actively selected favoriate folder.
  or top level if no folder is selected.

Copy link
Owner Author

@xiaoyifang xiaoyifang Feb 22, 2025

Choose a reason for hiding this comment

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

if (have group->folder association)
put word in the group's folder choice
else
put word in current actively selected favoriate folder.
or top level if no folder is selected.

Actually I want to remove this constraint in the future completely . Only leave put word in the current selected folder

@xiaoyifang xiaoyifang marked this pull request as ready for review February 22, 2025 00:51
@xiaoyifang xiaoyifang changed the title opt:favorite global data structure adjust opt:favorite icon adjustment,allow more situation Feb 22, 2025
@shenlebantongying
Copy link
Collaborator

4 states icons is way too complex. The fact that no suitable icon is there just speaks the problem itself.

Also see Firefox & Chrome's star icon, they only state that a page is bookmarked somewhere.

Copy link
Collaborator

@shenlebantongying shenlebantongying Feb 22, 2025

Choose a reason for hiding this comment

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

Not gonna to lie. This is not good.

It is hard to guess what this means without knowing the concept of "this folder" and "other folder". Even after knowing what it is, there is no good representation of the concept.

Also, Red is often associated with warnings. This would also be frustrating without knowing what this is (like the red dot notification in phone apps).

The original GD's "existing in this or other folder" concept is not that useful.

Let's just don't bring major confusion.

Copy link
Owner Author

@xiaoyifang xiaoyifang Feb 22, 2025

Choose a reason for hiding this comment

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

Also, Red is often associated with warnings

maybe change the style , blue?

users can learn :-)

@xiaoyifang
Copy link
Owner Author

4 states icons is way too complex. The fact that no suitable icon is there just speaks the problem itself.

Also see Firefox & Chrome's star icon, they only state that a page is bookmarked somewhere.

the chrome's favorite strategy is not good enough .
It can allow add the same page to different folders , while remove from the favorite ,it will remove them all.

@xiaoyifang xiaoyifang marked this pull request as draft February 22, 2025 03:16
@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Feb 22, 2025

How about we associate the icon with only the active folder and consider the top level as a folder too.

  • 🌟 -> word in current scope
    • click -> remove word to current active folder (and recrusively all subfolders)
  • ⚪ -> word not in current scope
    • click -> add word to current active folder
  • Add a new icon to indicate the active folder that is associated with the 🌟
  • Add a new action: Double click folder icon to make it the active folder
    • Edit: it is occupied by folding 😅
  • If old Group->Folder association exists, then set that folder to the active folder

This is perfect design

  • Simple by default -> Single top level
  • Powerful by choice -> Active certain folder
  • Compatible with legacy -> Old feature is enhanced
  • Consistent behavior -> Always add/remove word from the active folder

@xiaoyifang
Copy link
Owner Author

xiaoyifang commented Feb 22, 2025

How about we associate the icon with only the active folder

This is better ,and can be left in another PR. maybe first try to decouple the association between favorite and group

Current choice : associate group with favorite folder will cause much trouble ,such as this .
The word has been added to favorite ,but the icon will not reflect this. THis is what this PR has try to handle
image

image

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.

2 participants