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

feat: revise Favorites by introducing active folder to reduce ambiguity #2189

Merged
merged 11 commits into from
Feb 24, 2025

Conversation

shenlebantongying
Copy link
Collaborator

@shenlebantongying shenlebantongying commented Feb 22, 2025

Implement #2178 (comment)


Breakage / Enhancement:

Original GD's folder is ambiguous, group->folder mapping only maps the name.

a & b->a are considered the same.
|
| -> a
| -> b -> a
|    | -> c

Now requires the path fully specified like b/a for b->a folder and a is for folder a that is directly under top level only.

The group->folder is now only used when switching groups. Switching to a new group -> auto active folder of chosen.


Coding:

The central data is just a QStringList hat represents the nested folder.

Then various accompanying methods to utilize it.


Main idea: the 🌟 now associated with the active folder and consider the top level as the default folder.

  • Add a new icon to indicate the active folder that is associated with the 🌟
  • 🌟 -> word in
    • click -> remove word from current active folder.
  • ⚪ -> word not in
    • click -> add word to current active folder
  • 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

@shenlebantongying shenlebantongying force-pushed the active-fav-folder branch 2 times, most recently from 6eed09c to a9af107 Compare February 22, 2025 10:10
@xiaoyifang
Copy link
Owner

xiaoyifang commented Feb 22, 2025

issue 1:
group failed

goldendict_rqIIeXXzQj.mp4

issue 2:
the word is already in the folder,
image

@shenlebantongying shenlebantongying force-pushed the active-fav-folder branch 2 times, most recently from e2b865b to a48d899 Compare February 22, 2025 17:47
@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Feb 22, 2025

issue 1 is fixed.

Not sure about what issue 2 means, but I changed wording.

The Active is the new concept.

The original Add button need to revise:

  • in the context menu -> add to the current selected folder
  • in the main menu -> add to the active folder

The original GD's implementation is fusing main menu's fav and favPaneWidget's menu together.

Maybe just don't bring main menu's fav into the favPaneWidget's menu.

The main menu will have Export & Import & Add to active folder as operation on all items.

The context menu is editing the favPaneWidget including add to current selected folder.

@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Feb 22, 2025

BTW, the Hide is a duplication of View->Favoriate pane.

I just going to merge them.

Edit: history pane has the same issue.

@shenlebantongying shenlebantongying force-pushed the active-fav-folder branch 4 times, most recently from b74bd52 to 5b88148 Compare February 22, 2025 22:14
@shenlebantongying
Copy link
Collaborator Author

How about we rebrand this feature to “Bookmarks” to align with browsers.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Feb 23, 2025

How about we rebrand this feature to “Bookmarks” to align with browsers.

not the same senario, one deals with url while the other deals with word. maybe leave it for later discussion. not in this one PR

@xiaoyifang
Copy link
Owner

Not sure about what issue 2 means, but I changed wording.

issue 2,the word in already in the active folder, but the text is save favorite to this folder

@shenlebantongying shenlebantongying changed the title demo: implement "active fav folder" feat: revise Favorites by introducing active folder to reduce ambiguity Feb 23, 2025
@shenlebantongying
Copy link
Collaborator Author

This can be merged as is.

The favourites have various other issues unrelated to this feature.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Feb 23, 2025

issue 3:
image

when the folder a is not active , the add actually adds the word to the active folder. Which leads to misunderstanding.

I would suggest add to the current selected folder ,if there is none selected then add to the active folder. If there is no active folder neither, add to the root .

@xiaoyifang
Copy link
Owner

xiaoyifang commented Feb 23, 2025

issue 4;
image

The group's favorite folder is set to d/c/b/a , there is no folder is set active folder which IMO is right,then click the icon

image.

the word was added to root folder a.

the word should be added to root (consider there is no such favorite folder ) or create the actual folder d/c/b/a as it does not exist and then add the word

@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Feb 23, 2025

issue 3 is fixed by removing it for now. Need to decouple two menus #2189 (comment)

issue 4 require a manually created folder for now. (Part of issue 4 is fixed by early return)

They will be done next time (new PRs). #2194

@shenlebantongying
Copy link
Collaborator Author

The early return should be fixed now.


There is another issue: original GD automatically switching group when clicking word and the word is in user's group->folder association.

I think this is another design error, because when clicking other word without group->folder association. There is no way to know what group to switch back.

Anyway, I retain this design for now. And I added this to

Let's hear some feedbacks.

@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Feb 23, 2025

Also note that this feature is not recursive.

Consider this one:

if recursive for word `b`
Init state -> 🌟 -> click -> remove from `f2` -> 
 New state -> ⚪ -> click ->      add to `f1`
                                         ^ inconsistency here
This is less powerful, because recursive makes it hard to manage f1 without affecting f2

if not recursive for word `b`
Init state -> ⚪ -> click ->      add to `f1` -> 
 New state -> 🌟 -> click -> remove from `f1`
                                         ^ preditable here
This is more powerful because user can could add/remove word in f1 & f2 independently

.
├── a
├── f1  <- active
│   ├── f2
│   │   ├── a
│   │   ├── b
│   │
│   ├── c

@xiaoyifang
Copy link
Owner

xiaoyifang commented Feb 24, 2025

if not recursive for word `b`
Init state -> ⚪ -> click ->      add to `f1` -> 
 New state -> 🌟 -> click -> remove from `f1`
                                         ^ preditable here
This is more powerful because user can could add/remove word in f1 & f2 independently

I like this one ,when f1 is active or selected . selected should have more high priority than active. This can be left for further discusion or in #2194 .

return nullptr; // no match
}

QModelIndex FavoritesModel::getModelIndexByFullPath( const QStringList & fullPath ) const
Copy link
Owner

Choose a reason for hiding this comment

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

still a little complex , but this can do for now.

@xiaoyifang xiaoyifang merged commit 806ffac into xiaoyifang:staged Feb 24, 2025
10 checks passed
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.

Favorite: if word in a folder of favorite, use star_blue.svg instead of star.svg
2 participants