search

sugarlabs / sugar-web

Components for Sugar web activities
Apache License 2.0
13 stars 32 forks source link

Merge changes from Sugarizer's web library #133

Closed cheongsiuhong closed 4 years ago

cheongsiuhong commented 4 years ago

Part of #127 .

As per title, this pull request merges changes from Sugarizer's web library back to this repository.

Currently, running volo add -f from a Sugarizer activity causes it to pull sugar-web from this repository. However, this library is outdated and doesn't work with Sugarizer activities.

The changes accomplish the following:

  1. Works with Sugarizer activities.
  2. When running on Sugar Desktop, works similarly to the current repository. This is done with minimal changes, by using env.isSugarizer() to run different branches of code on the different platforms. In other words, the library is still essentially the same when running on Sugar Desktop.

What doesn't work:

  1. Sugarizer activities still will NOT work on Sugar Desktop with this library.
  2. grunt test is broken.

Most of the code written is by @llaske, with some fixes made by me during the porting.

The changes made are fairly extensive, and difficult to compare due to some files being moved around, particularly for datastore and bus. This is quite a major upheaval to this library, and I hope to get feedback from you guys.

chimosky commented 4 years ago

@cheongsiuhong can you share your test methods?

cheongsiuhong commented 4 years ago

I manually ran every Sugarizer activity with their default sugar-web, and this one. I noted no changes in behavior, though admittedly this is not the most thorough way to test.

chimosky commented 4 years ago

I manually ran every Sugarizer activity with their default sugar-web, and this one.

I'm guessing you ran them in sugar, if that's the case; how did you run it?

cheongsiuhong commented 4 years ago

I manually ran every Sugarizer activity with their default sugar-web, and this one.

I'm guessing you ran them in sugar, if that's the case; how did you run it?

The Sugarizer activities were tested in Sugarizer.

If you're asking how to run Sugarizer activities in Sugar, you can run python setup.py dev via the terminal. The corresponding activity should show up in Sugar desktop. Most of them don't work though, as mentioned.

chimosky commented 4 years ago

The Sugarizer activities were tested in Sugarizer.

Any changes made in sugar-web has to be tested with a web activity in sugar, a git search will show you the sugar-web activities.

The changes can't be tested on sugarizer activities in sugarizer as it's an update for sugar-web which is also used by sugar activities in sugar.

You can read more at the issue you referenced.

quozl commented 4 years ago

Nice work. Just need some way to test that everything is working in Sugar before we merge it. Is there a test method? Is it as simple as replacing the directory in an already working Sugar activity?

cheongsiuhong commented 4 years ago

Unfortunately, there are a fair number of issues that make this pull request somewhat hard to test properly.

The Karma tests don't work, probably due to the dependency being very outdated. As the purpose of this patch was solely to port changes over from Sugarizer, I avoided changing any dependencies.

Secondly, the Sugar web activities that are currently on the sugarlabs GitHub mostly use an outdated version of this library. It took some time, but I tested all of the activities here with this updated version of sugar-web. The Moon activity is the only one that uses a somewhat up to date sugar-web library, and it works with no issue when I replaced it with the updated one.

It is unclear if the problems lie with this patch, or with the outdated activities that may not be have been written to work with the current version (meaning unpatched) of sugar-web. It would take quite abit of time to debug and fix them all, if that were the case...

Hope to hear some guidance on this issue.

edit: some issues are caused by index.html calling sugar.css instead of the current sugar-200dpi.css.

chimosky commented 4 years ago

The challenges you've faced are expected, some problems might lie with the patch and some problems might not, you'll have to figure out which problems are which and try fixing them as you go along.

In the case of some activites index.html calling sugar.css and not the current sugar200dpi.css, that's a case of the patch not being the problem and you can easily fix that by referencing sugar200dpi.css and not sugar.css.

quozl commented 4 years ago

Thanks @cheongsiuhong and @chimosky.

I urge caution in using Moon as an example, because I made that release without much experience, and also how it can be used to induce a specific failure in the journal interaction.

Yes, the GitHub Sugarlabs repositories are outdated, because (a) we have had no JavaScript maintainers for Sugar, and (b) maintainers for Sugarizer have forked the source code into the Sugarizer repository. (Which has reminded me to pull from @llaske and push to @sugarlabs following the most recent release - done).

Sugar web has been under maintained. There is a 0.100 branch, a 0.102 branch, and then a v0.110.0 tag but no later. Sugar is at 0.117 now.

Guidance;

If you agree on this plan, then I suggest it be posted on sugar-devel@ mailing list, because the audience for this repository is limited.

llaske commented 4 years ago

@quozl I suggest also to update file one by one instead of updating all files at the same time. For example, update first env library, then bus library, then datastore ibrary, ...

When I've forked sugar-web I've isolated the initial source code an create sub directories for each file with the Sugarizer code in a sugarizer.js file and the Sugar code in a sugaros.js code. In the initial file name, there is only the code to switch between Sugar/Sugarizer.

For example, here is the code in the bus.js file: it's just a switch between sugaros.js file (where is the initial sugar-web code) and sugarizer.js file (where is the specific Sugarizer code).

define(["sugar-web/env", "sugar-web/bus/sugarizer", "sugar-web/bus/sugaros"], function(env, sugarizer, sugaros) {
    'use strict';

    var bus;
    if (env.isSugarizer()) {
        bus = sugarizer;
    } else {
        bus = sugaros;
    }
    return bus;
});

I can't guaranty that this logic has persisted since the beginning but with this, it should be easier to update Sugar-Web.

quozl commented 4 years ago

Thanks @llaske! Yes, that seems like it would be an effective way to iterate.

cheongsiuhong commented 4 years ago

Thanks all for the feedback.

Guidance;

  • select a Sugar web version for the next release, e.g. 0.118,
  • select some simple Sugar web activities, such as Hello World, and one or two others, chosen for their use of the API,
  • for each change to the API proposed by this pull request, make those changes also to the selected activities,
  • assess the changes that were required and write a "Port to Sugar web 0.118" guide for sugar-docs.

Currently, I think I'd work on the above steps. I agree with @llaske that pushing incrementally would make the updates easier to review, but it may be quite challenging to test the functionality of each iteration.

It would then require something like:

  1. Update a batch of files, eg datastore.js or env.js, and activity.js if needed.
  2. Test if some activities work, otherwise update them.
  3. Repeat 1 and 2 until fully updated.
  4. Release updated Sugar-web with porting tutorial.

Would this be a suitable workflow?

quozl commented 4 years ago

Thanks. That workflow seems fine to me. Another would be to add a test activity inside sugar-web or somewhere else that can serve as a worked example and test for the API. Like Hello World but using all API features.

cheongsiuhong commented 4 years ago

For work on incrementally updating sugar-web, I will open different pull requests for each separate file change.

Closed this for now.