lboehm commented 2 years ago

Hi @IObert,

I've found something and I'm not sure if it's an error:

What happens

when using easy-ui5 to generate a new project it generates the following view:

<mvc:View
    controllerName="com.myorg.myUI5App.controller.MainView"
    xmlns:mvc="sap.ui.core.mvc"
    displayBlock="true"
    xmlns="sap.m"
>
    <Shell id="shell">
        <App id="app">
            <pages>
                <Page id="page" title="{i18n>title}">
                    <content />
                </Page>
            </pages>
        </App>
    </Shell>
</mvc:View>

When adding another view it generates basically the same again (except the shell):

 <mvc:View controllerName="com.myorg.myUI5App.controller.DetailView"
  displayBlock="true"
  xmlns="sap.m"
  xmlns:mvc="sap.ui.core.mvc">
  <App id="DetailView" >
    <pages>
      <Page title="{i18n>title}">
        <content></content>
      </Page>
    </pages>
  </App>
</mvc:View>

As a result my UI5 app has n <App>-tags when it has n views. I'm not sure, but this doesn't seem right.

What I would expect

From my understanding it would be better to generate one "container file" that holds the app:

// App.view.xml

<mvc:View controllerName="a41s.workbook.controller.App" xmlns:html="http://www.w3.org/1999/xhtml" xmlns:mvc="sap.ui.core.mvc" displayBlock="true"
    xmlns="sap.m">
    <App id="app">
    </App>
</mvc:View>

And then, for each view generated a file like this one is generated:

<mvc:View 
    xmlns:uxap="sap.uxap" >
    <uxap:ObjectPageLayout>
        // ...
    </uxap:ObjectPageLayout>
</mvc:View>

The rootView (in my example the App.view.xml) would be defined in the manifest.json:

    "sap.ui5": {
        "rootView": {
            "viewName": "myorg.myapp.App",
            "type": "XML"
        },

This approach is described here: Step 2: Enable Routing

Am I missing something? Are there reasons why easy-ui5 isn't following this approach?

Thank you and best regards, Lukas

lboehm commented 2 years ago

It seems like we have here the same problem as in #20. As generator-ui5-project is using @sap-ux/fiori-freestyle-writer this can't be fixed in this project.

Any ideas how to solve that?

Best regards, Lukas

IObert commented 2 years ago

You are technically right here. It doesn't make much sense to have multiple sap.m.App controls in the same application. Initially, the idea here was that the generator cannot know which control is desired for a new view anyway, so I just went down the easy route and re-use the template I already had from the "default view". You could make a point that the subgenerator should just use another view and prompt the user for this. Tbh, I'm not sure if this would be very helpful but we can discuss this if you want. What would be your suggestion?

PS: #20 needs to be fixed first, I agree. But let's see if we find a good solution for this issue before we think about the implementation.

lboehm commented 2 years ago

Hi @IObert,

from a user perspective I would like the generator to behave like this:

When running the subgenerator for a new view, I would expect exact the same question as I get right now (like view name, create corresponding controller, etc.)
After the subgenerator is finished I would expect that the newly created view is generated correctly, so that there's only one <App> tag. As a user I want to use the created view without worrying about changing manifest.json or anything else. I just want to start writing my XML code in the view or my JS code in the controller.

From a technical perspective, I think the changes needed to achieve this behaviour are not so big:

We should add an App view and its corresponding controller. Then we would have to adapt the MainView.view.xml file and the manifest.json, so they are working together with the app view:

Adding App.view.xml:

<mvc:View controllerName="<%=namespace%>.<%=projectname%>.controller.App" xmlns:html="http://www.w3.org/1999/xhtml" xmlns:mvc="sap.ui.core.mvc" displayBlock="true" xmlns="sap.m">
<App id="app">

</App>
</mvc:View>

Adding App.controller.js:

sap.ui.define(
[
    "<%=namespace%>/<%=projectname%>/controller/BaseController", 
],
function (BaseController) {
    "use strict";

    return BaseController.extend("<%=namespace%>.<%=projectname%>.controller.App", {
    onInit() {
    },
    });
}
);

Changing MainView: We could remove the <Shell> and <App> tags, so it only holds the <Page> tag and its children

Adapt manifest.json, so it "knows" that App.view.xml is the app container:

    "sap.ui5": {
        "rootView": {
            "viewName": "myorg.myapp.App",
            "type": "XML"
        },

After this change, easy-ui5 can generate views and corresponding controllers and add them to the manifest.json.

I think it's a small effort to change the npm packages, but it brings huge benefits, because users don't have to adapt the manifest.json themselves. Especially for beginners this is quite error-prone.

What do you think?

BR, Lukas

IObert commented 2 years ago

It sounds to me that you basically rename MainView to App and just remove the Shell control and the <pages> aggregation.

Let's include @petermuessig here: Do you think this is feasible for this generator here?

lboehm commented 2 years ago

Hi @IObert,

no I wouldn't rename MainView to App. I would add App and change MainView, so it works together with App. If you like I could provide in the next few days a small git repo with a sample project. Describing this possible solution in just one issue is not that easy, as a few files are affected.

BR, Lukas

IObert commented 2 years ago

Yes, that would be helpful to understand. Thanks

lboehm commented 2 years ago

Hi @IObert,

I've created a sample repo with different branches. For each scenario I've created an PR inside my sample repo, so you can check what I've changed.

The master branch is just an app that was generated with easy-ui5.
The new-structure-initial branch is my proposal how easy-ui5 should generate an initial app.
- You can view the changes in this pull request. It's really not that much.
And when easy-ui5 is generating apps as I propose, you may wonder what easy-ui5 would have to generate to add new views to an existing project. The new-structure-detail-view branch shows exactly that.
- You can view the changes in this pull request. (Nevermind the formatter.js, I should update easy-ui5 as vobu provided a fix for that already 😃)
- Basically it's no difference to what easy-ui5 generates today. Just the structure of the newly generated view should be different, because we don't need the <App> tag (see line 4 in the DetailView file)

Does this help?

I really think that the changes to easy-ui5 are minimal. But the benefit is huge!

BR, Lukas

IObert commented 2 years ago

First of all, thanks for doing the work to create this comparison. I get the suggested change now :).

I can also see that this makes things a little bit easier to develop larger apps that require multiple pages etc. But it also makes simple hello world apps a bit more complicated as there are more files to handle and understand.

As this is not an urgent topic, I don't want to make the final call here. I'd suggest that we leave this issue open for a while to give others the opportunity to chime in here or to vote (👍 = change the template as instructed; 👎 keep it as is). Does this sound good?

jbm1991 commented 2 years ago

Thanks for doing that work @lboehm!

As a regular user of easy-ui5 I definitely support your suggestion.

I do take your point though @IObert that hello-world apps become slightly more complicated, but I think that is okay because it is a concept that anyone learning UI5 needs to understand early on. I also wonder how many people who are so new to UI5 that they are making hello-world applications, even know that easy-ui5 exists. So for me the priority should be making the lives easier of everyone else, rather than the completely new beginners in this case.

lboehm commented 2 years ago

Hi @IObert,

I get your point as well, that hello-world apps become slightly more complicated. But especially as a beginner it helps if a correct structure is given. If I would start with UI5 and I knew about easy-ui5, I would use easy-ui5 to generate the structure. If easy-ui5 would generate a structure that isn't aligned with best practices, I think this would confuse me as a beginner.

I like the approach to let the community vote. So let's see what the community thinks about this issue :)

BR, Lukas

IObert commented 2 years ago

9 upvotes already "indicate" a clear preference, that's great! I'd wait until tomorrow to "close" this vote but I think we can assume that the vote will be in favor of this proposal 😅

marianfoo commented 2 years ago

Funnily enough, this discussion was my problem this week when I started with a new app and wanted to add new pages and I didn't know what is current "best practice" right now. My way so far has always been to explore and learn a framework on a basic Hello World application. I agree with @lboehm that this clearly shows me how to proceed with further pages as a beginner and advanced user.

vobu commented 2 years ago

when implementing/changing the view template, please also look at the basic template of @sap-ux/fiori-freestlye-writer → it would make sense to have a single point of maintenance for best practice Views and Controllers

IObert commented 2 years ago

I think the vote cleariy went in favor of this suggestion. Thanks for everyone who shared their opinion here!

Let's talk about the implementation. As this project contains the templates for XML, JS, JSON, and HTML views, it makes sense to implement this (at least partially) in this repo. But @vobu is right, we should also try to get this improvement in the open-ux-tools. @tobiasqueck Do you think that would be easy thing to change in the Fiori Tools?

tobiasqueck commented 2 years ago

@IObert yes, this shouldn't be a problem. I had a look at our freestyle templates (https://github.com/SAP/open-ux-tools/tree/main/packages/fiori-freestyle-writer/templates) and for worklist and listdetail we already use the app-view -> page-view concept, therefore, also using it for the basic app makes a lot of sense purely from a consistency perspective even if it makes the basic app slightly more complex.

@petermuessig any objections or further inputs from your side? Once you give a thumbs up, I would create an issue in the open-ux-tools repo to get it done.

petermuessig commented 2 years ago

Sorry for being late to the party again. 😢

IMO, changing the generator to create additional views without an <App> just with a <Page> makes fully sense. The main view should always have the <App>. As @lboehm mentioned, this is the way it is documented in the Demo Kit to create views with a page only.

From my side also: 👍

tobiasqueck commented 2 years ago

Created https://github.com/SAP/open-ux-tools/issues/214. @lboehm (or anyone else) if you like, you can pick it up, otherwise, someone from the tools team will pick it up as soon as time permits.

lboehm commented 2 years ago

Hi @IObert & @tobiasqueck,

I wanted to solve this issue and I digged into it. These are the things I've found:

Peter mentioned above, that the best practice is that the MainView has the <App> . That's exactly what open-ux-tools is generating. So if I understood it correctly, there's no need to change the open-ux-tools.
To generate correct XML views we only have to change the content of the newly generated view, so the newly generated views don't have a <App> tag.

➡ This is what my PR does: #29

Fixing this issue for the other view types (HTML, JS, JSON) is a little bit trickier. For those view types the views aren't generated by open-ux-tools. Instead the newwebapp generator calls the newview generator, which is also called when the subgenerator for creating a new view is started. So we have two different scenarios with different requirements calling the same subgenerator. I see two possibilites to solve this issue:

We could make the newview generator more intelligent, so it checks if it's generating a whole new project. In that case it should generate a view, that has the <App>. And if it's called from a subgenerator it should generate a view that doesn't have an <App>.
Or we could add a view with an <App> to the newwebapp generator instead of calling the newview generator. Then we could change the newview generator so it's only generating views without <App>.

Maybe we should talk about, how to solve this issue for those views.

Summary

Nevertheless, I think my PR could be merged, because:

It solves this issue for XML views, which is propably by far the most used view type.
It doesn't change the behaviour of the generator for other view types, so this PR doesn't have any negative impact

So for most of the users the problem is solved, and for a small number the generator behaves the same. And when we found the right way to handle those other views we can adapt them afterwards.

What do you think?

Best regards, Lukas

tobiasqueck commented 2 years ago

@lboehm I think the open-ux-tools doing it correctly for most of the freestyle templates, however, the basic template only generates one view with shell and pages included: https://github.com/SAP/open-ux-tools/blob/main/packages/fiori-freestyle-writer/templates/basic/custom/View.xml. My understanding from this discussion was that it should be split into two: an app view and a page view. We are doing this already correctly for the other templates e.g. list-details: app (https://github.com/SAP/open-ux-tools/blob/main/packages/fiori-freestyle-writer/templates/listdetail/add/webapp/view/App.view.xml) and page (https://github.com/SAP/open-ux-tools/blob/main/packages/fiori-freestyle-writer/templates/listdetail/add/webapp/view/List.view.xml) but not for the basic template

lboehm commented 2 years ago

@tobiasqueck Yes, that was my understanding as well. But Peter wrote:

IMO, changing the generator to create additional views without an <App> just with a <Page> makes fully sense. The main view should always have the <App>.

I interpret the last sentence as follows: the MainView that is generated by open-ux-tools should hold the <App> and its own <Page> as well. So no need for an extra App.view.xml.

Or am I interpreting this information wrong?

tobiasqueck commented 2 years ago

Now I am also not sure anymore. @petermuessig we need you ...

IObert commented 2 years ago

Maybe we should talk about, how to solve this issue for those views.

I'd rather tend to the second option, as this will make life easier once open-ux-tools support templates for other view types as well. But I should share here that this is only my personal opinion and I won't merge any PRs as I'm no longer working for SAP.

petermuessig commented 2 years ago

Hi @lboehm , @tobiasqueck , @IObert ,

I need to be careful what I put on the digital paper 😁 . With main view, I really meant the App.view.xml. IMO, it is good to split the <App> and the <Page>s into separate views. Otherwise it also gets a bit confusing with the routing. So, I would recommend to initially create an App.view.xml hosting the <App> and an additional Main.view.xml to host the main <Page>.

App.view.xml: https://ui5.sap.com/#/entity/sap.m.tutorial.walkthrough/sample/sap.m.tutorial.walkthrough.36/code/webapp/view/App.view.xml

Overview.view.xml (=> Main.view.xml): https://ui5.sap.com/#/entity/sap.m.tutorial.walkthrough/sample/sap.m.tutorial.walkthrough.36/code/webapp/view/Overview.view.xml

Cheers, Peter

vobu commented 2 years ago

I won't merge any PRs as I'm no longer working for SAP.

but I will and am open to any kind of 🍺 -bribery, muahaha

(i see myself out, please continue the proper technical discussion)

lboehm commented 2 years ago

@tobiasqueck Okay, perfect. I think we have clarity now. The open-ux-tools have to be adapted. Maybe I will find some time over the holidays.

@IObert Sad, to hear you're leaving SAP. But good luck on your way 🙂 Hope you will be part of the UI5 universe nevertheless!

But who's in charge of merging now?

petermuessig commented 2 years ago

👀 - maybe me? Please be patient with me and in case of not responding in time just use the @petermuessig to make me aware of it...

@IObert : honestly, you can still stay by my side here... 😄

IObert commented 2 years ago

I think @nicogeburek will also be able to hit "merge" if he likes the proposal.

lboehm commented 2 years ago

@petermuessig & @nicogeburek

Okay, now that we have discussed the technical stuff, I think we can go back to the discussion if this PR can be merged.

As I wrote above:

Nevertheless, I think my PR could be merged, because:

It solves this issue for XML views, which is propably by far the most used view type.

It doesn't change the behaviour of the generator for other view types, so this PR doesn't have any negative impact

And I think I can add another bullet point to that list:

As Peter clarified we need the App.view.xml. This will be fixed at open-ux-tools (Issue 214. My PR works fine with the current implementation of open-ux-tools and it will work with the upcoming implementation, when <App.view.xml> is generated. So my PR is independent of the open-ux-tools implementation and can be merged.

What do you guys think? Can we merge this PR for now?

Solving the issue for the other view types can be done later on.

Best regards Lukas

nicoschoenteich commented 2 years ago

Hi all,

thanks @lboehm for your effort. What you say makes sense and your PR looks good to me. Will merge it 👍🏻 I'm looking forward to the changes in the open-ux-tools so that we have the app-view & page-view structure.

BR, Nico

lboehm commented 2 years ago

@petermuessig @vobu @nicogeburek @tobiasqueck

Hi guys,

I hope it's okay, that I write you here.

The PR for fixing this issue in the open-ux-tools is open for quite some time now. I provided a solution, but I'm not able to fix the tests, given the time I have.

Is there any chance to give this PR a higher priority, so it will be finished by someone from SAP? I don't think there's much left to do.

I think easy-ui5 should generate the views according to the best practices. So this is a vital PR, from my point of view.

Maybe you can help finishing this PR 🙂 Thank you very much.

Best regards Lukas

tobiasqueck commented 2 years ago

@lboehm it has been merged. Thanks for your contribution!

ui5-community / generator-ui5-project

Do the views generated by easy-ui5 align with best practices? #21

What happens

What I would expect

Summary