Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fixes in latest release #2096

Merged
merged 4 commits into from
Jan 13, 2020
Merged

Bug fixes in latest release #2096

merged 4 commits into from
Jan 13, 2020

Conversation

foxman69
Copy link
Contributor

@foxman69 foxman69 commented Jan 3, 2020

Fixed a few bugs:

  1. You forgot to define the data-settings-max value in the html, it caused "Max number of schedules reached" alert message
  2. Because the change of the use of numSchedules you mistakenly used the defenition of the function instead of the value (Line 1089 in the js)
  3. You incremented the schedules variable even thou it already starts from 0...

@mcspr
Copy link
Collaborator

mcspr commented Jan 3, 2020

You forgot to define the data-settings-max value in the html, it caused "Max number of schedules reached" alert message

This does not have any effect?

espurna/code/html/custom.js

Lines 1814 to 1815 in 62ad7da

if ("schedules" === key) {
$("#schedules").attr("data-settings-max", value.max);

JsonObject &schedules = root.createNestedObject("schedules");
schedules["max"] = SCHEDULER_MAX_SCHEDULES;

Nice find about the func though! Function definition in expression is somehow passable issue for the js runtime :/ While iterating on that pr it somehow passed both local and codacy's eslint.

IDs seem to only be used for indexing and label ids? I don't fully understand the tabindex thing across the webui, because MDN points out that it should be extremely rare thing to manually change it all over the place. My guess is that it has something to do with the menu, but I wonder if that can be more generic (+ ported out for Vue PR, since there are hardcoded values too)

@foxman69
Copy link
Contributor Author

foxman69 commented Jan 4, 2020

I have checked about the data-settings-max again, after a factory reset it works correctly
maybe from some reason it didn't initialized in the websocket after the update, anyhow I think its still better to hard-code the attribute in the html to prevent synchronization problems.

I have added another fix in my last commit about another misuse of function as a variable, JS is just a very forgiving language when it comes to arithmetic operations on variables this is way it didn't pop as an error.

About the IDs as you mentioned its only for labeling, but still in all the other places in the code you start from 0 so it think its better keeping the convention.

The tabindex variable determine the focus order of the elements on the page when you press "tab" button on the keyboard, before the fix it caused the tabindex value to be NaN all the time.

@mcspr
Copy link
Collaborator

mcspr commented Jan 5, 2020

Hm. So if the whole scheduler ws packet never gets sent for whatever reason, only then we don't have max set. We would still have scheduler tab present, since schVisible key is sent separatly.
Have you checked the ws data log in the Network tab of web development tools when that happened? In that case, you'd overwrite the existing settings, since you never have parsed them to begin with and schedules panel should have 0 schedules?

I'd rather defaults stay in one place to more easily track them e.g. like html placeholder for wifi dns setting was 8.8.8.8 for some reason, while this was not the default value in code.

tabindex stuff is more of question as to why we need to manually do it in every function (not only scheduler) and can the menu be the only hard-coded one. But the fix is correct, no problem there.

@foxman69
Copy link
Contributor Author

foxman69 commented Jan 6, 2020

Unfortunately i haven't checked the log while it happen and i cannot reproduce it again:(

@mcspr mcspr merged commit e3887da into xoseperez:dev Jan 13, 2020
@mcspr mcspr added this to the 1.14.2 milestone Mar 13, 2020
@foxman69 foxman69 deleted the dev8 branch June 9, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants