Stops loading of blend-module when offset unavailable #654

Divy123 · 2019-01-11T11:52:05Z

Fixes #649

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

Divy123 · 2019-01-11T12:02:05Z

Divy123 · 2019-01-11T12:04:46Z

@jywarren please review.

harshkhandeparkar · 2019-01-11T13:07:16Z

@Divy123 I think you should not interact with the DOM directly form the library. Instead maybe you can send the error to the ui somehow and let the ui handle it?

harshkhandeparkar · 2019-01-11T13:10:24Z

Or you can get the number of steps in the ui and let or not let the user add beln module.

Divy123 · 2019-01-11T14:00:41Z

Well we have already provided an option as offset for user to handle.
Also if you have seen I have produced same output as input without making any other changes and produced one notification for user to know.
Rest user can always change an offset by the input option provided.
So I think providing an extra functionality for something already created is not worth.

harshkhandeparkar · 2019-01-11T14:07:36Z

But if the library is used without ui or in cli thn it is going to produce an error

harshkhandeparkar · 2019-01-11T14:09:29Z

Also if you want to add the notification then you can remove the #stepRemovedNotify css and add a class called notification-toast maybe.

Divy123 · 2019-01-11T14:29:32Z

Since the callback is invoked immediately if offset step is not available, I don't think there should be any problem I don't think there should be an error.
Yeah css can be implemented like that.

harshkhandeparkar · 2019-01-11T14:32:18Z

What if that notification is activated somehow and without cli the file doesnt find the notification and then goes crazy?

harshkhandeparkar · 2019-01-11T14:34:12Z

Oh ok the notification is only activated if in browser is true. My mistake sorry. If that is so then you can maybe add the notification function to a different file?

harshkhandeparkar · 2019-01-11T14:45:36Z

Maybe a util function which fires a custom event in the ui? The ui can listen for it and produce the appropriate notification?

harshkhandeparkar · 2019-01-11T15:18:54Z

@Divy123 the css should be a class not id. The notifications can be given specific ids by the function later.

harshkhandeparkar · 2019-01-11T15:20:19Z

The ids will have to be generated from the payload itself

Divy123 · 2019-01-11T15:27:16Z

Well I don't think there is any need of id(s) as of now . It may be done later if needed.
Lets wait for the core team to review.

Divy123 · 2019-01-11T15:30:49Z

@jywarren please review.

harshkhandeparkar · 2019-01-11T15:46:56Z

Ok On 11-Jan-2019 9:00 PM, "Slytherin" <notifications@github.com> wrote: @jywarren <https://github.com/jywarren> please review. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#654 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AhKOnyNJtXZuRTErVz3tqkdBLeq1rzfJks5vCK4qgaJpZM4Z7Uht> .

harshkhandeparkar · 2019-01-11T17:35:33Z

@Divy123 can you please change the CSS to .notification-styles as the function is going to create more than one element with the same id.

Divy123 · 2019-01-11T18:22:35Z

Sure @harshkhandeparkar . Thanks for your suggestions. 😄

harshkhandeparkar · 2019-01-11T18:47:16Z

@Divy123 I think

$('.notification-styles').length

will always be > 0 once the first notification has been shown.

Divy123 · 2019-01-11T18:59:07Z

Any thing you suggest @harshkhandeparkar ? I think adding class to css was misleading.
I don't think that should have been done.

harshkhandeparkar · 2019-01-11T19:03:19Z

Any thing you suggest @harshkhandeparkar ? I think adding class to css was misleading.
I don't think that should have been done.

No, we add class only for styling not for the notification which is why ids exist. For referring to a single element in the whole DOM

harshkhandeparkar · 2019-01-11T19:05:52Z

This is why we need to auto-generate ids for each notification as each of them has a different message but same styling.

Divy123 · 2019-01-11T19:20:15Z

One more thing I didn't find any problem in the code when class was not added.
Any idea @harshkhandeparkar ?
I found adding class unrequired.

harshkhandeparkar · 2019-01-11T19:22:35Z

One more thing I didn't find any problem in the code when class was not added.
Any idea @harshkhandeparkar ?
I found adding class unrequired.

That is because step removed notification and offset notification were not used together. It is going to create ambiguity even is both are given the same class or if both are given the same id. Therefore they have to be given the same class but different ids to refer to them individually without repeating any styles.

Divy123 · 2019-01-11T19:42:28Z

@harshkhandeparkar sure I will look into.

harshkhandeparkar · 2019-01-11T20:09:15Z

@Divy123 how about dynamically generating IDs based on input like maybe using .toLowerCase() so that no inconsistencies take place and same duplicated only if msg is different.

harshkhandeparkar · 2019-01-11T20:10:27Z

Or actually no.
Your solution is better as IDs will not be duplicated anyways

Divy123 · 2019-01-11T20:46:58Z

@jywarren the work is done. Please review.

jywarren · 2019-01-11T21:33:01Z

src/modules/Blend/Module.js

+        if (priorStep.output === undefined) {
+            this.output = input;
+            if(options.inBrowser)
+            produceNotification("Offset Unavailable","unavailable-offset");


ah, interesting! could we have a generic notification function that is passed to the UI code like this:

UI.notify('message');

Then the UI would know what to do with it -- the CLI ui would print it out, but the HTML ui would display it. Does that make sense? We'd have to make sure each UI has a .notify() function, and/or we'd have to check that it exists before using it.

Sure @jywarren that will be great to implement.

Could you please provide a bit more details?

So basically the UI we use is just one option -- we can pass in a different UI in the options when the sequencer is started up (see examples/index.html). Our default HTML ui is here: lib/defaultHtmlSequencerUi.js

So, we should think of how different UIs will handle notifications. The CLI is written here, and actually doesn't supply a UI, although really it should:

image-sequencer/index.js

Line 4 in cfdb543

sequencer = ImageSequencer({ ui: false });

Perhaps we should add a new item to the UI spec as described here: https://github.com/publiclab/image-sequencer/#creating-a-user-interface

We could say all UIs can supply an optional notify() or onNotify() handler. Does this make sense?

Yeah sure . Great changes . Will love to work on this.

@jywarren I have made the said changes. Please review.

harshkhandeparkar · 2019-01-14T13:27:40Z

@Divy124 I have another idea. You could use the same id for each notification toast but just change the text each time for each notification. How well would that work?

Divy123 · 2019-01-14T13:32:36Z

That's a good idea but since I have provided different id's to different types of notifications that would do the work as same type of notificattions won't display at same time.

harshkhandeparkar · 2019-01-14T14:02:16Z

Yes but if you do that like the way I suggested, it will be future proof.

harshkhandeparkar · 2019-01-14T14:04:35Z

Append a notification to the dom and change its innerText to the notification message. Everytime the function is triggered, the same element's text can be changed and then it can be animated.

harshkhandeparkar · 2019-01-14T14:10:07Z

No problem after that. No two elements with same id. No crowding of dom and no redundant styles.

Divy123 · 2019-01-14T14:29:47Z

The idea is of course nice but as I told you that it is not possible for two notifications to pop up simultaneously so it won't do any bad.
As of future we will make as per your idea but that depends when we need it.
It can be a good issue in itself when needed.
BTW thanks for your help.

jywarren

Just a few small changes requested, thank you!!!! Great work! 👍 🎉

jywarren · 2019-01-14T21:59:50Z

src/modules/Blend/Module.js

@@ -24,6 +24,12 @@ module.exports = function Dynamic(options, UI, util) {
        // save first image's pixels
        var priorStep = this.getStep(options.offset);

+        if (priorStep.output === undefined) {
+            this.output = input;
+            UI.onNotify('Offset Unavailable','offset-notification');


Ah, if we're going to use it like this, maybe best have it just be UI.notify(...), is that OK?

Yeah sure..

jywarren · 2019-01-14T22:00:56Z

index.js

@@ -69,6 +69,9 @@ if (program.saveSequence) {
      for (var output in step.info.outputs) {
        console.log("[" + program.step + "]: " + output + " = " + step[output]);
      }
+    },
+    onNotify: function(msg) {
+      console.log('\x1b[36m%s\x1b[0m','🌟  '+msg);


Does this overwrite the default in /src/ui/...? Just checking!

Not much though providing a beautiful version in console.log
An effort to look it more like a notification.

jywarren · 2019-01-14T22:02:01Z

examples/demo.css


 .savesequencemsg{
  display: none;
  text-align: center;
  }

-#stepRemovedNotification {
+#offset-notification,#remove-notification {


Could we make a generic reusable .notification class here?

We can still leave the ids for hiding/showing uniquely. But to have a generic style seems nice and then it can be reused for more things later.

Divy123 · 2019-01-14T23:10:44Z

@jywarren please review.

jywarren · 2019-01-16T22:41:26Z

This looks great. I think it's ready to be merged, and I'd like to ask if you're interested in developing a simple test for the UIs - to confirm that they have a notify() method. In the default, we may be able to check the console.log output? This could be a good place to start on developing UI tests!

Merging!

jywarren · 2019-01-16T22:42:06Z

Oh! Also, shall we update the READMEs to mention the notify() method?

Divy123 · 2019-01-17T10:25:38Z

Ya sure @jywarren I am working on it.

Divy123 · 2019-01-18T13:18:48Z

@jywarren I am almost done.
Just one thing if you can guide me how may I test the CLI interface notify() which is present in index.js file?

Stops loading of blend-module when offset unavailable

e48b558

Added improvements

af6fee7

Merge branch 'main' into stop-blending

92cfe4a

Divy123 added 2 commits January 12, 2019 00:00

Update demo.css

bac7bd2

Update notification.js

14b4c7e

Divy123 added 3 commits January 12, 2019 01:19

Update defaultHtmlStepUi.js

0e3e346

Update Module.js

27cc957

Update notification.js

c6f10c9

jywarren requested changes Jan 11, 2019

View reviewed changes

adds generic UI function to user-interface

0899dae

jywarren requested changes Jan 14, 2019

View reviewed changes

divy123 added 2 commits January 15, 2019 03:58

Added changes

90733ab

Resolved merged conflicts

8167652

jywarren merged commit 81719a9 into publiclab:main Jan 16, 2019

Stops loading of blend-module when offset unavailable #654

Stops loading of blend-module when offset unavailable #654

Conversation

Divy123 commented Jan 11, 2019 • edited Loading

Divy123 commented Jan 11, 2019

Divy123 commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

Divy123 commented Jan 11, 2019 • edited Loading

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

Divy123 commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

Divy123 commented Jan 11, 2019

Divy123 commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019 via email

harshkhandeparkar commented Jan 11, 2019 • edited Loading

Divy123 commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

Divy123 commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

Divy123 commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019 • edited Loading

Divy123 commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

harshkhandeparkar commented Jan 11, 2019

Divy123 commented Jan 11, 2019

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

harshkhandeparkar commented Jan 14, 2019 • edited Loading

Divy123 commented Jan 14, 2019

harshkhandeparkar commented Jan 14, 2019

harshkhandeparkar commented Jan 14, 2019 • edited Loading

harshkhandeparkar commented Jan 14, 2019

Divy123 commented Jan 14, 2019 • edited Loading

jywarren 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

Divy123 Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Divy123 commented Jan 14, 2019

jywarren commented Jan 16, 2019

jywarren commented Jan 16, 2019

Divy123 commented Jan 17, 2019

Divy123 commented Jan 18, 2019

Divy123 commented Jan 11, 2019 •

edited

Loading

Divy123 commented Jan 11, 2019 •

edited

Loading

harshkhandeparkar commented Jan 11, 2019 •

edited

Loading

harshkhandeparkar commented Jan 11, 2019 •

edited

Loading

harshkhandeparkar commented Jan 14, 2019 •

edited

Loading

harshkhandeparkar commented Jan 14, 2019 •

edited

Loading

Divy123 commented Jan 14, 2019 •

edited

Loading

Divy123 Jan 14, 2019 •

edited

Loading