8000 WIP: Add shape library plugin by rolandlo · Pull Request #6020 · xournalpp/xournalpp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

WIP: Add shape library plugin #6020

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

Draft
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

rolandlo
Copy link
Member
@rolandlo rolandlo commented Nov 4, 2024

As per #6008 (reply in thread) this is @MiltonBalaOffical's Quick shape library plugin from https://github.com/MiltonBalaOfficial/Quick-shape-library with a bit of polish. More of it is needed, although the plugin is already in a usable state.

@rolandlo rolandlo marked this pull request as draft November 4, 2024 18:21
@rolandlo rolandlo force-pushed the add-shape-library-plugin branch from 7251a52 to 7990087 Compare November 16, 2024 06:36
@rolandlo rolandlo force-pushed the add-shape-library-plugin branch from 7990087 to fa9020a Compare December 29, 2024 07:05
Copy link
Contributor
@atticus-sullivan atticus-sullivan left a comment

Choose a reason for hiding this comment

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

Just some comments I had when reading over this. Nothing severe though.

Oh and I really like idea of this plugin (didn't test it yet though). If there's anything I can do to help, let me know.

## Add Your Own Shape with "Extract_Stroke_Info"

1. **Draw your own shape** in Xournal++ and place it at the top left corner of the page.
*Note: The shape will be inserted at the same position. When you want to insert the sape if the page is smaller than the current one, you may not be able to see the inserted shapes.*
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: sape -> shape

Copy link
Contributor

Choose a reason for hiding this comment

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

Well we could also do this in the plugin (placing the icon at the top left corner by translating the coordinates) and then on insertion e.g. place the icon at the center of the page (or viewport if this is possible).

A bit sad that we currently don't have a way of getting something like pointer coordinates for plugins. This would then be going into the direction of making custom tools via plugins. But this is way outside of the scope of this plugin (maybe just something to keep in mind for the future).
Maybe related: #4703

if app.addToSelection then
app.addToSelection(refs)
else
print("Cannot add shape to selection because of missing API, consider upgrading Xournal++")
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this safeguard (eventhough most users won't see it as usually they probably won't run xournalpp from the console)

Comment on lines 81 to 86
file:write("}\n")
file:write("return strokesData")
file:write(" -- Return the strokesData table") -- End of strokesData table
Copy link
Contributor

Choose a reason for hiding this comment

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

is using multiline strings "better" here? (probably it doesn't really matter)

e.g.

file:write([[}
return strokesData -- Return the strokesData table]])

@rolandlo rolandlo force-pushed the add-shape-library-plugin branch from 46c91f8 to 31077d4 Compare February 8, 2025 06:57
@atticus-sullivan
Copy link
Contributor

I'd be interested in having a look once this reaches a somewhat final state (always not sure about the current status with this setting of this PR here and the remote repo too). So don't hesitate to shoot me a message here.

@rolandlo
Copy link
Member Author
rolandlo commented Feb 8, 2025

I'd be interested in having a look once this reaches a somewhat final state (always not sure about the current status with this setting of this PR here and the remote repo too). So don't hesitate to shoot me a message here.

There's still quite some work to be done. Feature parity with the remote version is not yet reached. I'll let you know once it's reasonably complete.

@rolandlo rolandlo force-pushed the add-shape-library-plugin branch 3 times, most recently from 267d926 to 52fb8ac Compare February 18, 2025 07:59
@rolandlo
Copy link
Member Author

The plugin now has most features built in that https://github.com/MiltonBalaOfficial/ShapeLibraryWithLgi has and adds some more. Installing it on the system (and adding shapes to a user folder) is not yet supported. Here is a demo:

ShapesLibraryDemo.mp4

@atticus-sullivan
Copy link
Contributor

Not sure how you fast you want to get this done now. Sadly I've got not too much time right now, I'll probably only able to give this a more detailed look in the end of next week.
If you want to wait that long, fine. If not, there's probably no special need to explicitly wait for my review.

@rolandlo
Copy link
Member Author

Not sure how you fast you want to get this done now. Sadly I've got not too much time right now, I'll probably only able to give this a more detailed look in the end of next week. If you want to wait that long, fine. If not, there's probably no special need to explicitly wait for my review.

It will take more time than that for sure. I won't have too much time to work on it in the next couple of days.

@rolandlo rolandlo force-pushed the add-shape-library-plugin branch from efda9b4 to 43234f8 Compare February 26, 2025 16:40
@rolandlo
Copy link
Member Author
rolandlo commented Mar 2, 2025

Now there are system categories (which are read only; they can be hidden via a check button) and user categories (which you can freely modify; they can also be hidden via a check button). The system categories are only read once, since they shouldn't change.

For the user categories the config.lua file is stored in app.getFolder("config"), the shapes in app.getFolder("data"). Those are ~/.config/xournalpp/plugin-settings/ShapeLibrary and .local/share/xournalpp/plugin-data/ShapeLibrary on Linux, respectively.

Some cleanup is still needed, but mostly all functionality is there now.
@atticus-sullivan @MiltonBalaOfficial Please try it out and give some feedback when you have time.

@MiltonBalaOfficial
Copy link

It's great to see the official plugin almost ready! I've taken a look at it, and here’s my feedback:

  1. When adding a new shape, why is the file name required alongside the shape name? It would be more convenient if the file name could be automatically generated from the shape name.

  2. Currently, users cannot add shapes to existing categories. If someone wants to add a file to the same type of category, they must create a matching user category. Would it be possible to store all files in a single location where the user has modification access? This would make it easier to manage and update shapes.

  3. Since the plugin has evolved significantly from its initial version, some comments in the code might be confusing for future developers working on upgrades. It might be helpful to review and refine them for clarity.

  4. I’m continuing to use the plugin and will provide more feedback as I explore it further. Thanks for making the official version available.

@rolandlo
Copy link
Member Author
rolandlo commented Mar 2, 2025

It's great to see the official plugin almost ready! I've taken a look at it, and here’s my feedback:

1. When adding a new shape, why is the file name required alongside the shape name? It would be more convenient if the file name could be automatically generated from the shape name.

I agree, that would simplify matters for the user. The plugin could generate a file name from the shape name and if the file already exists, it could modify the filename so that it becomes unique.

2. Currently, users cannot add shapes to existing categories. If someone wants to add a file to the same type of category, they must create a matching user category. Would it be possible to store all files in a single location where the user has modification access? This would make it easier to manage and update shapes.

Yes, it would be possible, but it would complicate things internally quite a bit, I fear. Maybe it's better to add an option to duplicate an existing (system) category. What do you think about that?

3. Since the plugin has evolved significantly from its initial version, some comments in the code might be confusing for future developers working on upgrades. It might be helpful to review and refine them for clarity.

Sure, also the README.md has to be refined.

4. I’m continuing to use the plugin and will provide more feedback as I explore it further. Thanks for making the official version available.

Thanks! By the way, one shape I can't find much use for is the Border Booklet (in the Page Decoration category). What page format is it supposed to be good for? I usually have A4 (portrait) and there it's too large to fit onto the page.

@MiltonBalaOfficial
Copy link

By the way, one shape I can't find much use for is the Border Booklet (in the Page Decoration category). What page format is it supposed to be good for? I usually have A4 (portrait) and there it's too large to fit onto the page.

Since it was designed for a specific 16:9 setup, it may not be generally useful. I think it should be removed from the official plugin, as it is user specific.

Maybe it's better to add an option to duplicate an existing (system) category. What do you think about that?

Yes, that will be fine. This is what I meant earlier but couldn't express clearly. After duplication the user might uncheck the system categories. If it is possible to auto-duplication and auto uncheck in the first use it would be great!

Copy link
Contributor
@atticus-sullivan atticus-sullivan left a comment

Choose a reason for hiding this comment

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

Do you use an "IDE" which can make sense of LuaLS annotations? If so, maybe it would make sense for this a bit larger plugin to add those annotations to the functions (together with some doc-comment like annotations)

Also note, that (as I mentioned) I'm really no one you should ask about UI stuff so I didn't really review the lgi_dialog.lua file (just wanted this to note).

I also added some bug reports (this way we have a thread for an organized conversation and can resolve the thread when the bug is fixed) which are not solved yet. Now I just tested the plugin a bit, but don't worry I also want to help here looking deeper into the bugs in order to fix them (just not today anymore)


Oh and I really didn't want to forget this, but still I did :/
I really love the shapes, nice job creating them @MiltonBalaOfficial (at least as far as I'm aware, this was you)

@@ -0,0 +1,4 @@
label {
color: orange;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tbh the orange looks a bit odd (but this is opinion based). Can't we somehow refer to the user's GTK Style here?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by the user's GTK style?

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing you set via a system dialog or e.g. lxappearance. Something like Adwaita or Arc-Darker.

@@ -0,0 +1,616 @@
local _M = {}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also as a user it was not intuitive that new User cat. are not directly shown when adding them via the UI (from a code viewpoint it might make sense of course, but we'd need to communicate this somehow).

But then this also might be a bug since after a restart (of xournalpp), the new User cat. was present, but when adding a new User cat. the old User cat. disappeared again.

SImilar: When (after a restart) trying to add a shape to one of the User cat., all the User cat. disappear again but is present (can even be inserted) after another xournalpp restart.

Doesn't this happen for you too?

Copy link
Member Author

Choose a reason for hiding this comment

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

That must be a regression. By the way ticking the "Show user cat" off and on makes the categories appear.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be fixed now with commit a2a6fa3

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe related (similar "trigger"):

Still at least for me:

  1. disable system cat -> no categories shown
  2. add a custom user cat
  3. results in system cats being shown again

Same when adding a new shape to a user cat with the system cats disabled

@atticus-sullivan
Copy link
Contributor

Also an idea from me: Inserting shapes is inserting individual strokes. As a result the user can modify the shape afterwards (which I find pretty cool btw). But this also means the user might not be able to select the individual shape anymore. The solution for grouping strokes currently is putting them on an individual layer.

Maybe adding a checkbox which leads to click on insert > new layer > insert strokes would be a nice addition. But then I'm not sure how this integrates with the current way of new shapes extending the current selection (which also is a nice feature)

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.

3 participants
0