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

Improved user flag handling #37

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

drossberg
Copy link
Contributor

  • User flags get the sha1 checksum of their image bitmap as UUID (if
    they don't already have one)
  • This way, they will be recognized, if it isn't their first appearance,
    and duplicate entries in the user flag toolbar will be avoided
  • Domain-specific flags can be provided with a custom installation in
    the flags/user directory

@drossberg
Copy link
Contributor Author

Please, consider this as a suggestion.

@insilmaril
Copy link
Owner

This looks cool, thanks! I'd like to give it a bit more thought, will need some time.

@insilmaril
Copy link
Owner

Finally I thought a bit about your idea (which I really like): There is no "userflags" directory - these flags are saved in maps, also the default map, if necessary. A flag is only saved there, if used at least once.

So to apply a UUID based on file content, this should be created when loading that flag: Flag::load in flag.cpp Until an ID is created, it should be set to an invalid UID in the constructor.

Then in Main::setupFlag we could check for existing IDs of userflags.

A minor problem might be, that identical flags might have different uses for different users - later I wanted to add options to change the name or maybe even keysequence of flags. Then unique IDs might be an issue. Not sure yet how to handle this.

At the moment your patch does not work yet - I can load two identical images and they will create two identical userflags.

@drossberg
Copy link
Contributor Author

No "userflags" directory:
That's right. This is a new feature. It's meant for domain/application specific installations where a special set of flags (e.g. Boolean operations) is needed. The default map isn't optimal to load them, although the .vym file can be hacked to not show them in the map.

Identical flags might have different uses:
I thought about adding the name to the bitmap where the checksum will be computed from, but rejected this idea, because I consider the image as the important part of the flag. However, the name could be still added to distinguish between different uses.

I can load two identical images...:
This version of the patch is "minimal invasive" and does not change the behaviour of Main::addUserFlag(). As the discussion moves on, I can improve the patch, e.g. move the computation of the UUID to Main::setupFlag() or Flag::load() (this also depends on if the name shall be considered).

@insilmaril
Copy link
Owner

**Userflags directory ** sounds useful, would be easier to maintain than a default map. Still would be interesting to store meta information somewhere, e.g. tooltip in various languages. Maybe later...

We then also would need a settings dialog and some doc, how this is supposed to work

** Identical flags ** Should be fine I guess

If you want to work this out, you are very welcome
(I'm more focusing on completely reworking the overall layout, when I find time these days)

@drossberg
Copy link
Contributor Author

I'll work on it, when I find time ;)

- User flags get the sha1 checksum of their image bitmap as UUID (if
they don't already have one)
- This way, they will be recognized, if it isn't their first appearance,
and duplicate entries in the user flag toolbar will be avoided
- Domain-specific flags can be provided with a custom installation in
the flags/user directory
@drossberg drossberg marked this pull request as ready for review September 18, 2023 15:10
@drossberg
Copy link
Contributor Author

I updated the code with the current develop branch and added a small section to the user documentation.

@insilmaril
Copy link
Owner

Hi Daniel, sorry didn't really look into this PR yet, still struggling to get develop into a usable shape (currently the relinking multiple branches area is a bit of a mess, but will become quite nice soon I hope). Once I got this stabilized, I am going to have another look here.

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