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

Rework the plugin system #1313

Merged
merged 20 commits into from
Apr 29, 2020
Merged

Rework the plugin system #1313

merged 20 commits into from
Apr 29, 2020

Conversation

mbasaglia
Copy link
Contributor

Implements most of the things mentioned here: #1309 (comment)

@ice0
Copy link
Collaborator

ice0 commented Apr 1, 2020

It's ok, what travis build is errored, i will fix it ASAP.

@ice0
Copy link
Collaborator

ice0 commented Apr 1, 2020

Ok, Travis build fixed. After rebase all should be fine.

@mbasaglia
Copy link
Contributor Author

I've added support for importers defined in plugins.

To test this you can use my plugin: http://mattia.basaglia.gitlab.io/tgs/downloads.html
It supports a couple different formats, probably the easiest to test is SVG.

@morevnaproject
Copy link
Member

@mbasaglia Thank you for an awesome addition! Please check my comments here - #1309 (comment) ^__^

@morevnaproject
Copy link
Member

I have tested this PR, here are my thoughts:

  1. We now have two "Import" menu commands:
    screenshot_001

I was unable to test properly "Import file..." command, because importer gave me an error when I made an attempt to import Lottie json file. But that's different issue.

I guess that "Import file..." opens imported file in a new tab (not adds to currently opened document). So, I think the meaning of it is more like "File" - "Open" command.

I think that we should merge "Import file..." and "Open" commands for better UX.

  1. Export command works perfectly, I have tested with html and json export. ^__^

@morevnaproject morevnaproject removed their assignment Apr 2, 2020
@mbasaglia
Copy link
Contributor Author

yeah it does work more like open, I'll move the code to that dialog.

to test import the easiest way is to create a simple drawing in synfig, export with "[TGS] SVG" and import the same file.

@morevnaproject
Copy link
Member

Thank you! I want to say that I am absolutely excited about this feature and which possibilities it opens for further development.

For example, a very common case when newbie user tries to import some image using "File" - "Open" command. Synfig give them error, because images intended to be added via "File" - "import" command. But newbie users are not aware about that and they often reach out to support asking what's wrong.

With new plugins system we can create a very simple plugin, which allows to open images via "File" - "Open" command. The plugin will do simple thing - it will create a new document with imported image. ^__^

@mbasaglia
Copy link
Contributor Author

mbasaglia commented Apr 2, 2020

I've pushed some changes that do that and also fixed some issues.

Copy link
Contributor

@rodolforg rodolforg left a comment

Choose a reason for hiding this comment

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

I believe it is the whole filepath, right?

@mbasaglia
Copy link
Contributor Author

Yea

@mbasaglia
Copy link
Contributor Author

CI fail seems unrelated to my changes

$ ./1-setup-osx-brew.sh

Checking adwaita-icon-theme...

==> Downloading https://homebrew.bintray.com/bottles/gnome-icon-theme-3.22.0.yos

Error: Failed to download resource "gnome-icon-theme"

Download failed: https://homebrew.bintray.com/bottles/gnome-icon-theme-3.22.0.yosemite.bottle.tar.gz

[...]

==> Downloading https://download.gnome.org/sources/adwaita-icon-theme/3.22/adwai

Error: Failed to download resource "gnome-icon-theme"

Download failed: https://download.gnome.org/sources/adwaita-icon-theme/3.22/adwaita-icon-theme-3.22.0.tar.xz

The command "./1-setup-osx-brew.sh" failed and exited with 1 during .

@AnishGG
Copy link
Member

AnishGG commented Apr 2, 2020

I have restarted the job. Sometimes the download fails because of network errors at the server.

@lgtm-com
Copy link

lgtm-com bot commented Apr 3, 2020

This pull request fixes 1 alert when merging ef1f20e into f584626 - view on LGTM.com

fixed alerts:

  • 1 for Variable defined multiple times

@morevnaproject
Copy link
Member

I've pushed some changes that do that and also fixed some issues.

Thank you! I will be able to test it on Tuesday. ^__^

@mbasaglia
Copy link
Contributor Author

By the way, as a side effect of this PR, #1252 should be much easier to fix as all that is required is to recreate / update the menu items when language is changed.
(Previously plugin names were loaded at startup and not using the correct language).

@morevnaproject
Copy link
Member

Hi! I did one more round of testing and found the following:

The Lottie plugin (which is only capable for export currently) for some reason is displayed in "File" - "Open" dialog. The plugin doesn't has any importers declared, so I guess it shouldn't be there. ^__^

(I do not have any other export/import plugins installed, besides Lottie, which is shipped with this version of Synfig).

screenshot_001

@morevnaproject
Copy link
Member

Also, I suggest to make "Open history" button inactive when any filetype handled by plugin is selected.
screenshot_003

@mbasaglia
Copy link
Contributor Author

Also, I suggest to make "Open history" button inactive when any filetype handled by plugin is selected.
screenshot_003

I'm not too familiar with Gtk(mm) and I wasn't able to find a signal for when the filter changes (doesn't help that the documentation isn't great) do you know how that's done?

https://developer.gnome.org/gtkmm/stable/classGtk_1_1FileChooserDialog.html

@rodolforg
Copy link
Contributor

As far as I can tell, you can't access the filter combobox (and its signals) in a normal way, only what filter is currently active.

So I think "Open history" should behave differently according to the selected filter – if it's possible.

Regarding the Gtkmm docs, I use it, but often I read the Gtk docs themselves, as they're more ordered and cleaner than mm counterpart.

@mbasaglia
Copy link
Contributor Author

As far as I can tell, you can't access the filter combobox (and its signals) in a normal way, only what filter is currently active.

So I think "Open history" should behave differently according to the selected filter – if it's possible.

Currently for plugins "open history" just acts like open.

So I guess it isn't possible to disable the open history button, even if it's possible to write the code that enables/disables based on the current filter, it can't be run when the filter changes if we can't hook to the combo signal.

@rodolforg
Copy link
Contributor

Can you check if until commit 6e6eee5 it still worked?

If so, try to git bisect from this commit on ;)

@lgtm-com
Copy link

lgtm-com bot commented Apr 18, 2020

This pull request fixes 1 alert when merging b133f7c into 2ed703e - view on LGTM.com

fixed alerts:

  • 1 for Variable defined multiple times

@mbasaglia
Copy link
Contributor Author

Found a fix

@mbasaglia
Copy link
Contributor Author

um, I re-based again. Any chance of this getting merged?

@ice0 ice0 merged commit 51ba106 into synfig:master Apr 29, 2020
@ice0
Copy link
Collaborator

ice0 commented Apr 29, 2020

Sorry. Was busy last days.

Merged. Thank you!

@mbasaglia
Copy link
Contributor Author

Cool, I'll be documenting the new stuff in the XML file on the wiki

@rodolforg rodolforg mentioned this pull request May 10, 2021
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.

5 participants