Added dialog service for retrieving info from users. Select Project Dialog. useDialogCallback hook

[tjcouch-sil]

Resolves #388

---

This change is 

[irahopkinson]

Thanks for all your good work on this.

It still feels like we are inappropriately using tab panel floats for dialogs. One of the issues with using floats is it puts an unnecessary burden on the window docking system (saving layouts and positions) for something that is inherently outside the docking system. Is there some reason we don't just use MUI Dialogs that are designed for purpose (including already modal)? Or are we just trying to get past Tech Demo without doing that work?

Reviewed 39 of 39 files at r1, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @tjcouch-sil)

---

extensions/src/hello-someone/hello-someone.ts line 319 at r1 (raw file):

    papi.webViews.onDidAddWebView((addWebViewEvent) => {
      if (addWebViewEvent.webView.webViewType === peopleWebViewType)
        logger.log(

Nit: use info.

Code quote:

.log(

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 90 at r1 (raw file):

      title: 'Select Hello World Project',
    }).current,
    'None' as string,

Nit: I'm pretty sure this doesn't need to be here since a literal string is a string.

Code quote:

 as string

---

src/extension-host/extension-host.ts line 12 at r1 (raw file):

import { getErrorMessage } from '@shared/utils/util';
import { CommandNames } from 'papi-shared-types';
import { startProjectLookupService } from '@extension-host/services/project-lookup.service.host';

This doesn't follow our filename convention. What are you trying to convey with this filename? What is a 'host' file type mean?

---

src/extension-host/services/papi-backend.service.ts line 26 at r1 (raw file):

  ExtensionStorageService,
} from '@extension-host/services/extension-storage.service';
import { ProjectLookupServiceType } from '@shared/services/project-lookup.service.model';

This doesn't follow our filename convention. What are you trying to convey with this filename? Would project-lookup-service.model work?

---

src/extension-host/services/papi-backend.service.ts line 29 at r1 (raw file):

import projectLookupService from '@shared/services/project-lookup.service';
import dialogService from '@shared/services/dialog.service';
import { DialogService } from '@shared/services/dialog.service.model';

This doesn't follow our filename convention. What are you trying to convey with this filename? Would dialog-service.model work?

---

src/renderer/components/dialogs/dialog-base.data.ts line 13 at r1 (raw file):

  Component?: (props: DialogProps) => ReactElement;
  /**
   * The default icon for this dialog. This may be overridden by the `DialogOptions.title`

Copy and paster error? Change to DialogOptions.iconUrl.

Code quote:

DialogOptions.title

---

src/renderer/components/dialogs/dialog-base.data.ts line 29 at r1 (raw file):

  minWidth?: number;
  /** The minimum height to which the dialog can be set */
  minHeight?: number;

BTW I'm wondering if this is such a good idea to put dialog sizing in the hands of the extension developer. Dialog sizing is really more a function of the design system and the viewport size. Minimums might be a good guide but maybe not specifying the initial size? Thoughts? Perhaps this is just an interim setting?

Code quote:

  /** The width and height at which the dialog will be loaded */
  initialSize: FloatSize;
  /** The minimum width to which the dialog can be set */
  minWidth?: number;
  /** The minimum height to which the dialog can be set */
  minHeight?: number;

---

src/renderer/components/dialogs/dialog-base.data.ts line 54 at r1 (raw file):

  submitDialog(data: TData): void;
  /**
   * Rejects the dialog request with hte specified message

typo

Code quote:

hte

---

src/renderer/components/dialogs/dialog-base.data.ts line 112 at r1 (raw file):

}

export function hookUpDialogService({

BTW since this function is exported it should probably have a JSDoc.

---

src/renderer/components/dialogs/select-project.dialog.tsx line 13 at r1 (raw file):

type SelectProjectDialogProps = DialogProps<string>;

const SELECT_PROJECT_DIALOG_TYPE = 'platform.selectProject';

BTW I wondering if we exported this from a *.model.ts file it could be included in papi and we wouldn't need to have the other 2 magic strings floating around.

---

src/renderer/components/dialogs/select-project.dialog.tsx line 18 at r1 (raw file):

  const [projects, isLoadingProjects] = usePromise(
    projectLookupService.getMetadataForAllProjects,
    useMemo(() => [], []),

BTW er, what does this empty memo do for us? Is this some React trick?

---

src/renderer/components/dialogs/select-project.dialog.tsx line 23 at r1 (raw file):

  return (
    <div className="select-project-dialog">
      <div>{prompt}</div>

BTW Since prompt is a string, I'm guessing that doesn't allow us to format the prompt with bold, italic, etc? What if prompt needs to be more complicated with paragraphs, diagrams, etc?

---

src/renderer/components/dialogs/select-project.dialog.tsx line 33 at r1 (raw file):

        </ProjectList>
      )}
    </div>

BTW not strictly part of this PR but I'm wondering if this dialog needs an explicit "Cancel" button rather than just relying on the panel Close button?

---

src/renderer/components/dialogs/select-project.dialog.tsx line 37 at r1 (raw file):

}

const SELECT_PROJECT_DIALOG: DialogDefinition<typeof SELECT_PROJECT_DIALOG_TYPE> = {

BTW I think this should just be camelCase since it's an object in place of a class instance rather than a genuine const value (such as SELECT_PROJECT_DIALOG_TYPE), i.e. change to selectProjectDialog.

Code quote:

SELECT_PROJECT_DIALOG

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 393 at r1 (raw file):

  // If there was an error loading the tab, we create an error tab. But we also want to throw here
  // so people know there was a problem.
  // TODO: Do we really want to create an error tab in the first place? Or maybe that should only

FYI: yes, we need a better way to handle this but outside this PR. Thanks for adding the comment.

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 404 at r1 (raw file):

/**
 * Function to call to add or update a webview in the layout

BTW this part of the description is redundant and can be removed to improve readability.

Code quote:

Function to call to 

---

src/renderer/components/docking/paranext-tab-title.component.tsx line 4 at r1 (raw file):

import logger from '@shared/services/logger.service';

type ParanextTabTitleProps = {

BTW this is an opportunity to perhaps label this PlatformTabTitleProps or simply TabTitleProps to reduce the later burden of renaming.

---

src/renderer/services/dialog.service.host.ts line 44 at r1 (raw file):

 * List of dialogs that were resolved or rejected from outside the dock layout and have not yet been
 * closed and rejected from inside the dock layout. The second reject should do nothing as it is an
 * expected extra call.

BTW why do we get a second close? Is this more funkiness around using tab panel floats or something else?

---

src/shared/services/dialog.service.ts line 8 at r1 (raw file):

async function initialize(): Promise<void> {
  if (!initializationPromise) {
    initializationPromise = (async () => {})();

Is this anticipating initialization that we know we need soon or is this currently YAGNI?

---

src/shared/services/dialog.service.model.ts line 14 at r1 (raw file):

   *
   * @returns returns the user's response
   * @throws if the user cancels

Throwing seems like the wrong thing to do when a user cancels a dialog. IMO throwing should be reserved for developer errors, not user selection. Here and below.

---

src/shared/services/web-view.service.ts line 56 at r1 (raw file):

  onLayoutChangeRef: MutableRefObject<OnLayoutChangeRCDock | undefined>;
  /**
   * Function to call to add or update a tab in the layout

BTW This seems redundant and can be removed to improve readability.

Code quote:

Function to call to 

---

src/shared/services/web-view.service.ts line 65 at r1 (raw file):

  addTabToDock: (savedTabInfo: SavedTabInfo, layout: Layout) => Layout | undefined;
  /**
   * Function to call to add or update a webview in the layout

BTW This seems redundant and can be removed to improve readability.

Code quote:

Function to call to 

---

src/shared/services/web-view.service.ts line 276 at r1 (raw file):

 * the persisted layout information and loads it into the dock layout.
 *
 * WARNING: YOU CANNOT USE THIS FUNCTION IN ANYTHING BUT THE RENDERER

BTW The warning uses a double negative which is harder to understand. State what you want rather than what you don't not want! e.g.:
"WARNING: ONLY USE THIS FUNCTION IN THE RENDERER"
Here and below in 3 other places.

---

src/renderer/components/project-components/download-update-project-tab.component.scss line 0 at r1 (raw file):
BTW This seems an overly verbose folder name. Replace project-components with projects.

---

src/renderer/components/project-components/project-list.component.tsx line 13 at r1 (raw file):

};

export function fetchProjects(): Project[] {

BTW this should state that it's mock data and the issue that will address this. This has been fixed on main.

[lyonsil]

Reviewed 32 of 39 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 34 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 90 at r2 (raw file):

  const [project, selectProject] = useDialogCallback(
    'platform.selectProject',
    useRef({

Can we come up with a way for this hook to work without requiring the caller to remember to call useRef? This design I assume means if someone forgets then the renderer will spin re-rendering. It feels fragile.

---

src/extension-host/extension-host.ts line 12 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This doesn't follow our filename convention. What are you trying to convey with this filename? What is a 'host' file type mean?

@irahopkinson - TJ and I were talking about this sort of naming. In the newer design we talked about, we're separating the "server" code that is only meant to run in a specific process from the "client" code that is meant to run in any process that wants to use the "server." We talked about naming that "server" code with host. Do you have a different idea for naming?

---

src/extension-host/services/papi-backend.service.ts line 26 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This doesn't follow our filename convention. What are you trying to convey with this filename? Would project-lookup-service.model work?

@irahopkinson - TJ and I were talking about this sort of naming. In the newer design we talked about, we're separating the "server" code that is only meant to run in a specific process from the "client" code that is meant to run in any process that wants to use the "server." I think it's worthwhile to talk about updating our naming guidelines to clarify exactly what we want to name these different files.

---

src/renderer/index.tsx line 19 at r2 (raw file):

webViewProviderService.initialize();
webViewService.initialize();
startDialogService();

Don't we need to await all of these calls? I know you're just following the pattern be adding another line that doesn't await, but it seems like we need to await these calls. For example, if the network service doesn't finish initializing before we run other services that use it, I'm not sure how well we will recover.

---

src/renderer/components/dialogs/dialog-base.data.ts line 27 at r2 (raw file):

  initialSize: FloatSize;
  /** The minimum width to which the dialog can be set */
  minWidth?: number;

I assume we're talking about units in terms of pixels and not points or picas or something else, but it would be good to call this out in the comment. Same below.

---

src/renderer/components/dialogs/dialog-base.data.ts line 104 at r2 (raw file):

/**
 * Reject a dialog request. Synchronously rejects, then asynchronously closes the dialog

What if the async closing of the dialog fails? With this pattern the error is lost. Why not make the call async?

---

src/renderer/components/dialogs/select-project.dialog.tsx line 18 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW er, what does this empty memo do for us? Is this some React trick?

And can we just have a default value for that argument instead of making callers use this syntax?

---

src/renderer/components/dialogs/select-project.dialog.tsx line 37 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I think this should just be camelCase since it's an object in place of a class instance rather than a genuine const value (such as SELECT_PROJECT_DIALOG_TYPE), i.e. change to selectProjectDialog.

My current understanding is that we should only use UPPER_SNAKE_CASE for constant strings, numbers, and booleans. Where are we on that?

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 239 at r2 (raw file):

  switch (layout.position) {
    case 'center':
      left = layoutSize.width / 2 - width / 2;

What if the dialog is larger than the layout? Could a dialog's top bar be offscreen and unselectable? Do we need to make sure that left and top have a minimum of 0?

---

src/renderer/hooks/papi-hooks/use-dialog-callback.hook.ts line 65 at r2 (raw file):

  // type assert to tell TS that undefined will be part of `TResponse` if `defaultResponse` is not
  // specified but is not otherwise
  const [response, setResponse] = useState(defaultResponse as TResponse);

Can the default response ever change?

---

src/renderer/hooks/papi-hooks/use-dialog-callback.hook.ts line 73 at r2 (raw file):

      setIsShowingDialog(true);
      try {
        // Looks like we need to assert here because it can't tell this is a TResponse. It can just

When I see "assert" and not "type assert", my brain immediately goes to the meaning of "assert" in C++, C#, Java, etc.. It would be helpful to me, at least, if we always say "type assert" or "assert the type". Alternately, if there is a another term that doesn't use the word "assert" that is fine, too.

---

src/renderer/hooks/papi-hooks/use-dialog-callback.hook.ts line 87 at r2 (raw file):

      }
    }
  }, [dialogType, options, isShowingDialog]);

I'm not a React expert here, but it seems like watching isShowingDialogin the callback while calling setIsShowingDialog here could cause React to loop. javascript - React: Updated state not reflecting in recursive useCallback - Stack Overflow has an example that seems similar. You might tell me I'm wrong, but it doesn't seem like this is going to do exactly what we want.

---

src/renderer/services/dialog.service.host.ts line 106 at r2 (raw file):

  // Close the dialog
  // We're not awaiting closing it. Doesn't really matter right now if we do or don't successfully close it

I'm really not comfortable explicitly avoiding calls to await to make things synchronous. It's a pattern that seems likely to cause unexpected problems eventually.

---

src/shared/services/dialog.service.ts line 8 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Is this anticipating initialization that we know we need soon or is this currently YAGNI?

If we aren't going to need an initialize, let's just drop it. This seems a little odd as-is.

---

src/shared/services/web-view.service.ts line 276 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW The warning uses a double negative which is harder to understand. State what you want rather than what you don't not want! e.g.:
"WARNING: ONLY USE THIS FUNCTION IN THE RENDERER"
Here and below in 3 other places.

I will be happy once we can rework this service so that process-specific portions are only available in those processes.

---

src/shared/services/project-lookup.service.model.ts line 1 at r2 (raw file):

import { ProjectMetadata } from '../models/project-metadata.model';

Minor nit: it would be nice if we made this a reference from @shared instead of using ...

[tjcouch-sil]

Using the existing pathways certainly reduces complications. It also theoretically will allow us one day to split P.B into a multi-window application more easily (UX might want dialogs in their own window like in PT9, though it seems they will discuss and get back to us). It also gives us a nice pathway to preserving dialogs between refreshes if we ever want to put the effort into making that happen.

Also, MUI dialogs' modality probably would need some work anyway since I imagine they don't have any recommended ways to synchronize modality between multiple windows as we will have one day. Not to say it wouldn't be a head-start; just that it would also need work.

These things being said, it's probably most that I just went with what I perceived to be a low-resistance path. We could always change things in the future. I would not say, though, that I intended to put this in just to get past tech demo.

Reviewable status: 11 of 45 files reviewed, 23 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

extensions/src/hello-someone/hello-someone.ts line 319 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Nit: use info.

Oops! Thanks :)

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 90 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Nit: I'm pretty sure this doesn't need to be here since a literal string is a string.

Actually unfortunately 'None' in this case is interpreted as type "None", not type string, probably because it's a hard-baked const string. I had to ponder the differences in the way TypeScript interprets specific strings vs string overall. I believe the difference is when TypeScript determines that the variable can change. So let thing = 'stuff' is type string, but const thing = 'stuff' is type "stuff".

So this needs to be type asserted here because of this issue (as mentioned in the JSDoc for useDialogCallback here). Basically I'm using the type of this parameter to determine if the returned result type can be undefined or not, and this is the consequence. :p not the greatest thing, but maybe people won't mind it being undefined in general and won't run into this much. Just not sure what the best approach was here, so I went with this.

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 90 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Can we come up with a way for this hook to work without requiring the caller to remember to call useRef? This design I assume means if someone forgets then the renderer will spin re-rendering. It feels fragile.

Yeah, I think I'd like to think of some way to improve this, too... Though I'd prefer to put that feedback in #552 and think about it then (I added it as a new bullet). This is following current patterns of our custom hooks. Maybe we ought to say options should never change unless you unmount and remount a component! But I don't want to do that here in just one place without a big-picture reflection and discussion. Maybe there's a better way, but I can't think of one currently.

---

src/extension-host/extension-host.ts line 12 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

@irahopkinson - TJ and I were talking about this sort of naming. In the newer design we talked about, we're separating the "server" code that is only meant to run in a specific process from the "client" code that is meant to run in any process that wants to use the "server." We talked about naming that "server" code with host. Do you have a different idea for naming?

This .service.host file describes the actual implementation on a process of a service as we deliberated over in #203 long ago. Matt implemented the first service, project-lookup.service.ts, with this pattern, then we discussed changing the name to fit our current patterns a bit better.

I suggested this naming scheme to promote file-name-search (Ctrl+P) discoverability:

In a call, we decided on .service-host, .service, and .service-model as it is apparently more consistent with our style guide.

(Sorry, Matt; I wrote this before you responded. Didn't mean to rewrite what you said)

---

src/extension-host/services/papi-backend.service.ts line 26 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This doesn't follow our filename convention. What are you trying to convey with this filename? Would project-lookup-service.model work?

This .service.model file describes the shape of a service. Matt implemented the first service from #203, project-lookup.service.ts, with this pattern of a separate interface file describing the service, then we discussed changing the name to fit our current patterns a bit better.

I suggested this naming scheme to promote file-name-search (Ctrl+P) discoverability:

I'd prefer to keep the name through .service intact so it is easy to search on file name and find all three relevant files. Do you have any thoughts?

---

src/extension-host/services/papi-backend.service.ts line 29 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This doesn't follow our filename convention. What are you trying to convey with this filename? Would dialog-service.model work?

See above.

---

src/renderer/index.tsx line 19 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Don't we need to await all of these calls? I know you're just following the pattern be adding another line that doesn't await, but it seems like we need to await these calls. For example, if the network service doesn't finish initializing before we run other services that use it, I'm not sure how well we will recover.

Each service that initializes internally awaits anything that the service depends on. Everything in the React stuff doesn't need the services to start in order to get to first paint, so we don't need to await these services. Most of these services (except the web view provider service; I think I probably just put it here because I had a lot on my mind and wanted to just have it spin up) are initialized here because they set up request handlers on the renderer process that other processes need to use.

We do need to strengthen our initialization sequence and our exception handling throughout the software at some point, but I would consider that to be far outside the scope of this issue.

---

src/renderer/components/dialogs/dialog-base.data.ts line 13 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Copy and paster error? Change to DialogOptions.iconUrl.

Yep! Thanks!

---

src/renderer/components/dialogs/dialog-base.data.ts line 29 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I'm wondering if this is such a good idea to put dialog sizing in the hands of the extension developer. Dialog sizing is really more a function of the design system and the viewport size. Minimums might be a good guide but maybe not specifying the initial size? Thoughts? Perhaps this is just an interim setting?

I'm not exactly sure what you're asking here, so I will clarify some things in case that helps us to know what we're talking about:

These three are part of the dialog definition, which is determined by the dialog creator and is not changeable when calling dialogs. Each dialog can have different sizes and such, but extension developers can't change the built-in dialog sizes.

However, developers can change these values for their own webviews (and conceivably their own custom dialogs whenever we get that in with #551). Are you suggesting they shouldn't be able to define their own sizes for these? I imagine they should be able to set up their webviews and dialogs to match their content size appropriately, but maybe we could specify absolute minimums that they can't go below. Any thoughts?

---

src/renderer/components/dialogs/dialog-base.data.ts line 54 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

typo

Thanks!

---

src/renderer/components/dialogs/dialog-base.data.ts line 112 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW since this function is exported it should probably have a JSDoc.

Oops! Good call.

---

src/renderer/components/dialogs/dialog-base.data.ts line 27 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I assume we're talking about units in terms of pixels and not points or picas or something else, but it would be good to call this out in the comment. Same below.

Good call. Done.

---

src/renderer/components/dialogs/dialog-base.data.ts line 104 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

What if the async closing of the dialog fails? With this pattern the error is lost. Why not make the call async?

I don't know what we would want to do if it failed (which seems rather unlikely and like a larger problem than just this one dialog request), so I figured might as well make the important parts synchronous so the things that need to happen when this is called from React happen and leave out the rest for now. I thought we could make a synchronous call to close the tab if we end up deciding to make synchronous dockLayout calls from the web view service available (which I kinda hope we do before long since it seems like it would help with a lot of things).

I'll make an IIFE here so we log more contextual info in case this does ever happen to be a problem. Let me know if you have any thoughts on what else you think we should do if anything.

---

src/renderer/components/dialogs/select-project.dialog.tsx line 13 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I wondering if we exported this from a *.model.ts file it could be included in papi and we wouldn't need to have the other 2 magic strings floating around.

I'm not a huge fan of this either... I suppose we could change it as suggested, but it breaks from our current pattern with commands and such where we have mapped types that work directly from const strings. I definitely appreciate a desire to change them to use const references, though... But that would require putting them on the actual PAPI, not just on the papi.d.ts, and we would have to figure out letting extensions add their own unless we wanted our internal ones using consts on the PAPI and everyone else's using magic strings. Any thoughts? Maybe we could put them on a CachedDataProvider? Seems like a lot of work for a bunch of strings, but maybe it's worth haha I dunno

---

src/renderer/components/dialogs/select-project.dialog.tsx line 18 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

And can we just have a default value for that argument instead of making callers use this syntax?

Yeah, unfortunately the way I wrote usePromise, it re-runs the function every time the default state is changed, so even the default state has to be a stable reference. So wrapping the empty array in a useMemo makes it a stable reference. We could actually probably put it in a useRef, but it probably isn't particularly impactful. I made an issue #552 to revise these hooks like making the default state not reactive so we don't have to do weirdness like this.

The default if the argument is unspecified is undefined. I just didn't want to account for that value in this context, so I made it [].

---

src/renderer/components/dialogs/select-project.dialog.tsx line 23 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW Since prompt is a string, I'm guessing that doesn't allow us to format the prompt with bold, italic, etc? What if prompt needs to be more complicated with paragraphs, diagrams, etc?

I'd say we should probably make some larger issue around rich text editing. It would have impacts on a lot of things including our Project Notes data api and such. Would be good to nail this down at some point.

Created #557

---

src/renderer/components/dialogs/select-project.dialog.tsx line 33 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW not strictly part of this PR but I'm wondering if this dialog needs an explicit "Cancel" button rather than just relying on the panel Close button?

I imagine we could maybe make some kind of dialog frame inside DIALOG_BASE that wraps the dialog and provides stuff like a cancel button so it's easy to share between dialogs. Maybe another property each dialog can define that lets them add their own React component in the buttons area? I just didn't want to worry about that yet 😂 want to make an issue for it?

---

src/renderer/components/dialogs/select-project.dialog.tsx line 37 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

My current understanding is that we should only use UPPER_SNAKE_CASE for constant strings, numbers, and booleans. Where are we on that?

If I freeze it, does that make it any better? :) (actually I'm going to go freeze it now because it should be frozen haha oops) In my mind, frozen reference consts seem pretty much equivalent to primitive type consts (and they're both runtime consts unlike in many other languages), but I wasn't really sure what would be best. I believe the naming convention specifications are not specific enough to clearly dictate what to do in this situation, and I believe we are already inconsistent throughout our code base regarding this convention. It's probably best to consult the team on this to figure out what will help us best as a whole. Could we add a line to the style discussions about this rather than figuring out here?

Also, interestingly, I think our style guide as it is written right now dictates that we make all constants PascalCase because that's the C# convention and there is nothing stating otherwise. I don't know that we're doing this anywhere on the node side. Oops haha oh well.

---

src/renderer/hooks/papi-hooks/use-dialog-callback.hook.ts line 65 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Can the default response ever change?

The argument defaultResponse can be passed in differently across different renders, certainly. However, I documented in its JSDoc that it is used at the very beginning and never again. When you call to show a dialog, it doesn't reset the value to the default value. I thought maybe someone might want that in the future, so I made a little comment about where to do that if we want to implement that maybe in a hook options parameter (as opposed to the current dialog options parameter haha) some day.

---

src/renderer/hooks/papi-hooks/use-dialog-callback.hook.ts line 73 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

When I see "assert" and not "type assert", my brain immediately goes to the meaning of "assert" in C++, C#, Java, etc.. It would be helpful to me, at least, if we always say "type assert" or "assert the type". Alternately, if there is a another term that doesn't use the word "assert" that is fine, too.

Sounds good. Done

---

src/renderer/hooks/papi-hooks/use-dialog-callback.hook.ts line 87 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I'm not a React expert here, but it seems like watching isShowingDialogin the callback while calling setIsShowingDialog here could cause React to loop. javascript - React: Updated state not reflecting in recursive useCallback - Stack Overflow has an example that seems similar. You might tell me I'm wrong, but it doesn't seem like this is going to do exactly what we want.

The dependency array for useCallback is used to create a new callback function, not to run the function itself like in a useEffect. Setting a state in a callback that is listening for changes to that state is fine; it will just (appropriately) create a new function with the updated state on the next render. If we had a useEffect here, we would either not want to do this or establish some base case like in normal recursion. Preferably avoid doing it somehow 😂

---

src/shared/services/dialog.service.ts line 8 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Is this anticipating initialization that we know we need soon or is this currently YAGNI?

Oh I should have awaited the network service here. Good catch! Thanks!

---

src/shared/services/web-view.service.ts line 56 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW This seems redundant and can be removed to improve readability.

Done

---

src/shared/services/web-view.service.ts line 65 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW This seems redundant and can be removed to improve readability.

Done

---

src/shared/services/web-view.service.ts line 276 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I will be happy once we can rework this service so that process-specific portions are only available in those processes.

Done.

Yes, I will also be very happy. This is messy.

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 404 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW this part of the description is redundant and can be removed to improve readability.

Good call

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 239 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

What if the dialog is larger than the layout? Could a dialog's top bar be offscreen and unselectable? Do we need to make sure that left and top have a minimum of 0?

That would be a wise thing to work out in another issue regarding float size/position limits for all tabs as this is a big conversation. Since before this PR, you have been able to set webviews to whatever size you want, and they can totally block the screen and prevent you from doing anything. There's also some complication regarding what to do if the window is so small that even reasonable tabs and dialogs don't fit.

---

src/renderer/components/docking/paranext-tab-title.component.tsx line 4 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW this is an opportunity to perhaps label this PlatformTabTitleProps or simply TabTitleProps to reduce the later burden of renaming.

Sure! Sounds good

---

src/renderer/components/project-components/download-update-project-tab.component.scss line at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW This seems an overly verbose folder name. Replace project-components with projects.

Done

---

src/renderer/components/project-components/project-list.component.tsx line 13 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW this should state that it's mock data and the issue that will address this. This has been fixed on main.

I don't see a fix on main - I think I would have hit a merge conflict if so. Do you know where that comment is so I can keep things consistent?

I went ahead and added one for the time being.

---

src/renderer/services/dialog.service.host.ts line 44 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW why do we get a second close? Is this more funkiness around using tab panel floats or something else?

Yep, it's in the onLayoutChange callback on the dock layout, and it gets run when you click the X on the dialog window. Now that I think about it, I probably should have made a function to run to check for the current dialog requests available to resolve/reject... Yeah, looks simpler. Done.

---

src/renderer/services/dialog.service.host.ts line 106 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I'm really not comfortable explicitly avoiding calls to await to make things synchronous. It's a pattern that seems likely to cause unexpected problems eventually.

Responded above in dialog.service.model.ts

---

src/shared/services/dialog.service.model.ts line 14 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Throwing seems like the wrong thing to do when a user cancels a dialog. IMO throwing should be reserved for developer errors, not user selection. Here and below.

I went back and forth on this, but I decided in the end that I'd leave the data itself completely up to the dialog and have throw for when the user cancels. That way, if you use useDialogCallback, for example, you can define a default value and never have to worry about the type being anything but what you have decided it should be. I guess that doesn't seem to be worth all that much, though, so how about null if the user cancels so dialogs have undefined available to their definitions? Then we could make null just set back to the default value inside the hook!

Done

---

src/shared/services/project-lookup.service.model.ts line 1 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Minor nit: it would be nice if we made this a reference from @shared instead of using ...

Done

[tjcouch-sil]

Reviewable status: 11 of 45 files reviewed, 23 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

src/extension-host/services/papi-backend.service.ts line 26 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This .service.model file describes the shape of a service. Matt implemented the first service from #203, project-lookup.service.ts, with this pattern of a separate interface file describing the service, then we discussed changing the name to fit our current patterns a bit better.

I suggested this naming scheme to promote file-name-search (Ctrl+P) discoverability:

I'd prefer to keep the name through .service intact so it is easy to search on file name and find all three relevant files. Do you have any thoughts?

Oh, sorry, I forgot to update this discussion. We concluded in making all these .service-whatever.ts

[irahopkinson]

Reviewed 7 of 7 files at r2, 32 of 34 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

src/renderer/components/dialogs/dialog-base.data.ts line 29 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'm not exactly sure what you're asking here, so I will clarify some things in case that helps us to know what we're talking about:

These three are part of the dialog definition, which is determined by the dialog creator and is not changeable when calling dialogs. Each dialog can have different sizes and such, but extension developers can't change the built-in dialog sizes.

However, developers can change these values for their own webviews (and conceivably their own custom dialogs whenever we get that in with #551). Are you suggesting they shouldn't be able to define their own sizes for these? I imagine they should be able to set up their webviews and dialogs to match their content size appropriately, but maybe we could specify absolute minimums that they can't go below. Any thoughts?

What if the viewport is smaller than the initialSize? I'll leave off discussing this as probably what I was initially thinking of is out of scope here and fits in #551.

---

src/renderer/components/dialogs/dialog-base.data.ts line 119 at r4 (raw file):

/**
 * Set the functionality of submitting and canceling dialogs. This should be called specifically by
 * `dialog.service.host.ts` immediately on startup and by nothing else. This is only here to

Change to service-host now. There might be a bunch of these renames in comments you should search for.

Code quote:

service.host

---

src/renderer/components/dialogs/select-project.dialog.tsx line 13 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'm not a huge fan of this either... I suppose we could change it as suggested, but it breaks from our current pattern with commands and such where we have mapped types that work directly from const strings. I definitely appreciate a desire to change them to use const references, though... But that would require putting them on the actual PAPI, not just on the papi.d.ts, and we would have to figure out letting extensions add their own unless we wanted our internal ones using consts on the PAPI and everyone else's using magic strings. Any thoughts? Maybe we could put them on a CachedDataProvider? Seems like a lot of work for a bunch of strings, but maybe it's worth haha I dunno

Yeah, that larger discussion is out of scope here. But you could remove one magic string by declaring and exporting this in dialog-definition.model.ts.

---

src/renderer/components/dialogs/select-project.dialog.tsx line 18 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Yeah, unfortunately the way I wrote usePromise, it re-runs the function every time the default state is changed, so even the default state has to be a stable reference. So wrapping the empty array in a useMemo makes it a stable reference. We could actually probably put it in a useRef, but it probably isn't particularly impactful. I made an issue #552 to revise these hooks like making the default state not reactive so we don't have to do weirdness like this.

The default if the argument is unspecified is undefined. I just didn't want to account for that value in this context, so I made it [].

Ok. Do you get a stable reference if an empty array const is declared outside the funtion definition?

---

src/renderer/components/dialogs/select-project.dialog.tsx line 23 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'd say we should probably make some larger issue around rich text editing. It would have impacts on a lot of things including our Project Notes data api and such. Would be good to nail this down at some point.

Created #557

Err, #557 doesn't seem to cover what I'm getting at since a dialog prompt doesn't need to be editable rich text. If prompt was of typestring | JSX.Element then we could format the prompt. However, a valid answer to my first question could be "we are keeping it simple as a string for now".

---

src/renderer/components/dialogs/select-project.dialog.tsx line 37 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

If I freeze it, does that make it any better? :) (actually I'm going to go freeze it now because it should be frozen haha oops) In my mind, frozen reference consts seem pretty much equivalent to primitive type consts (and they're both runtime consts unlike in many other languages), but I wasn't really sure what would be best. I believe the naming convention specifications are not specific enough to clearly dictate what to do in this situation, and I believe we are already inconsistent throughout our code base regarding this convention. It's probably best to consult the team on this to figure out what will help us best as a whole. Could we add a line to the style discussions about this rather than figuring out here?

Also, interestingly, I think our style guide as it is written right now dictates that we make all constants PascalCase because that's the C# convention and there is nothing stating otherwise. I don't know that we're doing this anywhere on the node side. Oops haha oh well.

Freezing it does it for me.

I'm not seeing what you are seeing in the Code Style Guide. The only mention of "the C# convention" is in the C#-specific Guidelines section which doesn't apply to TS code. However, I do agree that the TypeScript-specific Guidelines are not clear on what to do in this case.

---

src/renderer/components/docking/paranext-tab-title.component.tsx line 4 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Sure! Sounds good

Err.. I was just indicating for this type only rather than introducing another thing that needs to be renamed. Thanks for the much bigger rename though since it does need to happen at some point - even if out of scope here.

---

src/renderer/components/project-components/project-list.component.tsx line 13 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I don't see a fix on main - I think I would have hit a merge conflict if so. Do you know where that comment is so I can keep things consistent?

I went ahead and added one for the time being.

My mistake, I thought Jolie's PR was merged to main already but it hasn't yet. This is what I was thinking of but actually not related like I was recalling.

Thanks for the comment.

---

src/renderer/services/dialog.service.host.ts line 44 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Yep, it's in the onLayoutChange callback on the dock layout, and it gets run when you click the X on the dialog window. Now that I think about it, I probably should have made a function to run to check for the current dialog requests available to resolve/reject... Yeah, looks simpler. Done.

Nice improvement.

[lyonsil]

Lots of good stuff here. I just have one lingering concern.

Reviewed 33 of 34 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 90 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Yeah, I think I'd like to think of some way to improve this, too... Though I'd prefer to put that feedback in #552 and think about it then (I added it as a new bullet). This is following current patterns of our custom hooks. Maybe we ought to say options should never change unless you unmount and remount a component! But I don't want to do that here in just one place without a big-picture reflection and discussion. Maybe there's a better way, but I can't think of one currently.

Ok - I added some thoughts to that ticket.

---

src/renderer/index.tsx line 19 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Each service that initializes internally awaits anything that the service depends on. Everything in the React stuff doesn't need the services to start in order to get to first paint, so we don't need to await these services. Most of these services (except the web view provider service; I think I probably just put it here because I had a lot on my mind and wanted to just have it spin up) are initialized here because they set up request handlers on the renderer process that other processes need to use.

We do need to strengthen our initialization sequence and our exception handling throughout the software at some point, but I would consider that to be far outside the scope of this issue.

I don't think it's outside the scope of this issue to add await to the front of initializing functions we're adding onto. As a general practice we ought to be awaiting all async functions without good reason. If you really don't want to await these, can you add an explicit comment saying why we aren't awaiting?

---

src/renderer/components/dialogs/dialog-base.data.ts line 104 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I don't know what we would want to do if it failed (which seems rather unlikely and like a larger problem than just this one dialog request), so I figured might as well make the important parts synchronous so the things that need to happen when this is called from React happen and leave out the rest for now. I thought we could make a synchronous call to close the tab if we end up deciding to make synchronous dockLayout calls from the web view service available (which I kinda hope we do before long since it seems like it would help with a lot of things).

I'll make an IIFE here so we log more contextual info in case this does ever happen to be a problem. Let me know if you have any thoughts on what else you think we should do if anything.

Thanks, I think that's a reasonable step to take. I agree this is an unusual and unlikely situation, but those are exactly the kinds of things that require extra logging or other checking because if they happen they will be baffling without extra data.

---

src/renderer/components/dialogs/select-project.dialog.tsx line 37 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Freezing it does it for me.

I'm not seeing what you are seeing in the Code Style Guide. The only mention of "the C# convention" is in the C#-specific Guidelines section which doesn't apply to TS code. However, I do agree that the TypeScript-specific Guidelines are not clear on what to do in this case.

Ok, I'm probably crossing C# patterns here. Freezing makes sense to me.

---

src/renderer/hooks/papi-hooks/use-dialog-callback.hook.ts line 87 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

The dependency array for useCallback is used to create a new callback function, not to run the function itself like in a useEffect. Setting a state in a callback that is listening for changes to that state is fine; it will just (appropriately) create a new function with the updated state on the next render. If we had a useEffect here, we would either not want to do this or establish some base case like in normal recursion. Preferably avoid doing it somehow 😂

Thanks for the clarification!

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 239 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

That would be a wise thing to work out in another issue regarding float size/position limits for all tabs as this is a big conversation. Since before this PR, you have been able to set webviews to whatever size you want, and they can totally block the screen and prevent you from doing anything. There's also some complication regarding what to do if the window is so small that even reasonable tabs and dialogs don't fit.

Sounds good - opened #558 to track going forward.

[tjcouch-sil]

Reviewable status: 40 of 45 files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 90 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Ok - I added some thoughts to that ticket.

Gotcha. Thanks!

---

src/renderer/index.tsx line 19 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I don't think it's outside the scope of this issue to add await to the front of initializing functions we're adding onto. As a general practice we ought to be awaiting all async functions without good reason. If you really don't want to await these, can you add an explicit comment saying why we aren't awaiting?

I'll put an IIFE on each like we agreed on in the other case. Seems like an easy quick fix that we can both be happy with.

I also filed #559

---

src/renderer/components/dialogs/dialog-base.data.ts line 29 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

What if the viewport is smaller than the initialSize? I'll leave off discussing this as probably what I was initially thinking of is out of scope here and fits in #551.

Oh actually Matt brought up the same concern! He filed #558 on it. Thanks!

---

src/renderer/components/dialogs/dialog-base.data.ts line 104 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Thanks, I think that's a reasonable step to take. I agree this is an unusual and unlikely situation, but those are exactly the kinds of things that require extra logging or other checking because if they happen they will be baffling without extra data.

Gotcha. Thanks!

---

src/renderer/components/dialogs/dialog-base.data.ts line 119 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Change to service-host now. There might be a bunch of these renames in comments you should search for.

Done. Thanks!

---

src/renderer/components/dialogs/select-project.dialog.tsx line 18 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Ok. Do you get a stable reference if an empty array const is declared outside the funtion definition?

Yep! I'm just lazy 😂

---

src/renderer/components/dialogs/select-project.dialog.tsx line 37 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Ok, I'm probably crossing C# patterns here. Freezing makes sense to me.

Oh yeah, you're right - I thought I remembered we adhered to C# conventions in all cases where we don't state otherwise even in TypeScript code, but I see now it says "C#-specific". Thanks!

Regardless, if we want to have a discussion (not part of this PR) to see what communicates best to our team, that works for me. I wasn't totally sure on this point, and it seems there isn't a particular consensus. Seems it's team-by-team.

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 239 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Sounds good - opened #558 to track going forward.

Awesome. Thank you!

[lyonsil]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

src/renderer/index.tsx line 19 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'll put an IIFE on each like we agreed on in the other case. Seems like an easy quick fix that we can both be happy with.

I also filed #559

Thanks!

---

src/renderer/components/dialogs/select-project.dialog.tsx line 37 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Oh yeah, you're right - I thought I remembered we adhered to C# conventions in all cases where we don't state otherwise even in TypeScript code, but I see now it says "C#-specific". Thanks!

Regardless, if we want to have a discussion (not part of this PR) to see what communicates best to our team, that works for me. I wasn't totally sure on this point, and it seems there isn't a particular consensus. Seems it's team-by-team.

In C#, generally I only saw UPPER_SNAKE_CASE used for compile-time constants. There isn't an equivalent in TS/JS, but I think a constant that cannot be changed after it is set is probably close.

Agree this doesn't hold up the current PR.

[tjcouch-sil]

Reviewable status: 42 of 45 files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

src/renderer/components/dialogs/select-project.dialog.tsx line 13 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Yeah, that larger discussion is out of scope here. But you could remove one magic string by declaring and exporting this in dialog-definition.model.ts.

Done

---

src/renderer/components/dialogs/select-project.dialog.tsx line 23 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Err, #557 doesn't seem to cover what I'm getting at since a dialog prompt doesn't need to be editable rich text. If prompt was of typestring | JSX.Element then we could format the prompt. However, a valid answer to my first question could be "we are keeping it simple as a string for now".

I'd like to touch base and get in sync on this, but let's leave it for another time if that's ok - I would indeed like to keep it simple for now.

[irahopkinson]

Reviewed 4 of 5 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved