Update app-model documentation #325

lucyleeow · 2024-01-18T05:26:42Z

Description

Updates the app-model documentation:

Fleshes out existing information and adds napari specific information.
Add guide on testing app-model
Add details on app-model vs action manager implementation differences

This depends on napari 4991 as I link to a file in the repo that is created in that PR.

lucyleeow · 2024-01-18T05:27:01Z

ping @DragaDoncila @jni who may be interested

DragaDoncila

@lucyleeow thanks for getting started on this - so much useful information in here. I know it's still in draft but I've done a first pass and left mostly grammar/spelling fixes as well as a couple of other suggestions. Feel free to ignore if you have a different workflow!

DragaDoncila · 2024-01-24T06:54:37Z

docs/developers/architecture/app_model.md

@@ -2,66 +2,195 @@

 # napari's application model

-```{important}
-**This is not a part of the public napari interface!**


@lucyleeow I think it's probably still good to keep this notice here? Afaik there's no public interface for the app?

No, but it is now inside the 'architecture' section which only includes docs aimed at devs, so we can infer that it is not part of the interface. I guess it's good to spell it out.

But as I understand, there is intention to make it public:
napari/napari#6054

Without making get_app public, the actions cannot be simply called from plugins.

If we do make it public we can amend this message.

I think the intention is more to allow users to register actions at runtime. Depending on the contribution type, the user may already have a public API func to access plugin actions- e.g., there is the open_sample func for sample contributions, and I think there may be a way for the user to get reader/writers too?

I think that option to call actions is as important, as runtime registration

You think users should call a command in app via the command ID string instead of using existing public API like open_sample? Why?
Or do you just mean you want users to have the option of calling a command via string?

Because we provide public API only for opening sample and adding plugin widget. And plugins could provide many more contributions.

And there is an area for people who would like to provide custom controllers (either widget or hardware)

Sure but these are not the current comtribution types? We could create public API for new contribution types. Have we already decided how these new contributions would look?

I'm not opposed to execute via app model but there's probably a few decisions to be made here and this way may not be the best?

Yeah I don't think this is going to get hashed out in this PR lol.

Let's update the message to: This is not currently a part of the public napari API, but we are considering ways to expose action execution directly.

Once we actually decide what the interface for that is, we will amend.

docs/developers/architecture/app_model.md

lucyleeow · 2024-01-25T11:24:09Z

Okay I think this is ready for review now. Thanks @DragaDoncila for the thorough review already!

psobolewskiPhD

Sorry this fell through the cracks!
It's really good and useful, I did catch a few broken links, so requesting changes, but otherwise it's good to go IMO.

docs/developers/architecture/app_model.md

psobolewskiPhD · 2024-03-22T21:28:56Z

docs/developers/architecture/app_model.md

+## app-model vs action manager implementation differences
+
+App-model and action manager differ in when actions are executed.
+In app-model, actions are defined executed when they


actions are defined executed when

This is confusing to me, either one of those should be dropped (executed?) or there is an "and" missing?

Oh yeah I mucked that up. I think I mean 'executed'? (because define just means which file they are written in right??). When a function is connected to an event or registered to the app, the function itself is 'run' right? I mean we import the e.g., view menu actions and that import runs the functions inside the file where the view menu actions are defined?!

What is the best phrasing here?

cc @DragaDoncila

I would not use executed in this context. I would say "defined" when referring to the file they are literally written in, and registered when they are added to the app-model/action_manager registry.

Yeah good point. I've remembered what I was thinking about when I wrote this -> plugins menu actions used to be registered later when we create the plugins menu (when we know we have Qt). Now we register plugin actions much earlier, when we don't know if we have Qt. (Thus we've had to do 'safe register')

Done @DragaDoncila, let me know what you think, thanks!

psobolewskiPhD

Looks good to me, I'll let @DragaDoncila & @Czaki make the final call on the two open questions.

DragaDoncila

Left a few comments to unresolved conversations. @lucyleeow ping me once these are done and I'll approve!

DragaDoncila · 2024-03-25T06:40:54Z

docs/developers/architecture/app_model.md

@@ -2,66 +2,195 @@

 # napari's application model

-```{important}
-**This is not a part of the public napari interface!**


Yeah I don't think this is going to get hashed out in this PR lol.

Let's update the message to: This is not currently a part of the public napari API, but we are considering ways to expose action execution directly.

Once we actually decide what the interface for that is, we will amend.

DragaDoncila · 2024-03-25T06:43:36Z

docs/developers/architecture/app_model.md

+## app-model vs action manager implementation differences
+
+App-model and action manager differ in when actions are executed.
+In app-model, actions are defined executed when they


I would not use executed in this context. I would say "defined" when referring to the file they are literally written in, and registered when they are added to the app-model/action_manager registry.

DragaDoncila

Looks good to me! Let's get this in and we can always improve in future PRs. Thanks @lucyleeow 🙏

lucyleeow · 2024-03-27T03:28:29Z

@psobolewskiPhD could this go in? 🙏 I've got a new PR that depends on it 😬

# References and relevant issues Follow on from #378 Depends on #325 # Description Add cross reference to other testing info in `testing.md` (waiting on #325 so I can add cross ref label) --------- Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

first additions

3ae23bb

lucyleeow marked this pull request as draft January 18, 2024 05:26

github-actions bot added the documentation Improvements or additions to documentation label Jan 18, 2024

github-actions bot assigned lucyleeow Jan 18, 2024

lucyleeow added content Ideas for new or improved content and removed documentation Improvements or additions to documentation labels Jan 18, 2024

lucyleeow added 2 commits January 22, 2024 15:31

Merge branch 'main' into appmodel

edf886a

pp docs

16c742c

github-actions bot added the documentation Improvements or additions to documentation label Jan 23, 2024

testing docs

2d74339

DragaDoncila reviewed Jan 24, 2024

View reviewed changes

lucyleeow added 6 commits January 25, 2024 14:40

wip context

dc233c9

review

2139c0f

mock app wording

decf785

keybinding reword

5a3f9ab

context;

3ae8300

vs act manager

e8cdc9d

lucyleeow marked this pull request as ready for review January 25, 2024 11:23

lucyleeow added 3 commits January 27, 2024 10:30

Merge branch 'main' into appmodel

7af9a8c

Merge branch 'main' into appmodel

0cf963b

minor amendment

bf6b76d

lucyleeow mentioned this pull request Feb 9, 2024

Shapes Convert to Labels layer list contextual menu is greyed out for new layer, need to reselect the layer napari/napari#6643

Closed

Merge branch 'main' into appmodel

1d84e93

psobolewskiPhD requested changes Mar 22, 2024

View reviewed changes

Czaki added this to the 0.5.0 milestone Mar 22, 2024

lucyleeow added 3 commits March 23, 2024 10:10

review

a7aaabd

Merge branch 'appmodel' of github.com:lucyleeow/docs into appmodel

a32bb97

add mock app note

b797a96

psobolewskiPhD approved these changes Mar 23, 2024

View reviewed changes

DragaDoncila reviewed Mar 25, 2024

View reviewed changes

lucyleeow and others added 2 commits March 26, 2024 11:46

review

9966d36

Update docs/developers/architecture/app_model.md

2ecf8f8

DragaDoncila approved these changes Mar 26, 2024

View reviewed changes

lucyleeow added the ready to merge label Mar 26, 2024

lucyleeow mentioned this pull request Mar 27, 2024

Add cross reference to other testing info in testing.md #379

Merged

DragaDoncila merged commit c5ab377 into napari:main Mar 27, 2024
6 checks passed

lucyleeow deleted the appmodel branch March 27, 2024 05:07

psobolewskiPhD removed the ready to merge label Apr 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update app-model documentation #325

Update app-model documentation #325

lucyleeow commented Jan 18, 2024 •

edited

Loading

lucyleeow commented Jan 18, 2024

DragaDoncila left a comment

DragaDoncila Jan 24, 2024

lucyleeow Jan 25, 2024

Czaki Mar 22, 2024

lucyleeow Mar 22, 2024

Czaki Mar 22, 2024

lucyleeow Mar 22, 2024

Czaki Mar 23, 2024

lucyleeow Mar 23, 2024

DragaDoncila Mar 25, 2024

lucyleeow commented Jan 25, 2024

psobolewskiPhD left a comment

psobolewskiPhD Mar 22, 2024

lucyleeow Mar 22, 2024 •

edited

Loading

DragaDoncila Mar 25, 2024

lucyleeow Mar 25, 2024

lucyleeow Mar 26, 2024

psobolewskiPhD left a comment

DragaDoncila left a comment

DragaDoncila Mar 25, 2024

DragaDoncila Mar 25, 2024

DragaDoncila left a comment

lucyleeow commented Mar 27, 2024

Update app-model documentation #325

Update app-model documentation #325

Conversation

lucyleeow commented Jan 18, 2024 • edited Loading

Description

lucyleeow commented Jan 18, 2024

DragaDoncila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyleeow commented Jan 25, 2024

psobolewskiPhD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyleeow Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psobolewskiPhD left a comment

Choose a reason for hiding this comment

DragaDoncila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DragaDoncila left a comment

Choose a reason for hiding this comment

lucyleeow commented Mar 27, 2024

lucyleeow commented Jan 18, 2024 •

edited

Loading

lucyleeow Mar 22, 2024 •

edited

Loading