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

Restart with confirmation #1663

Merged
merged 3 commits into from
Nov 9, 2023
Merged

Conversation

mkz212
Copy link
Contributor

@mkz212 mkz212 commented Nov 9, 2023

show modal with confirmation, like after click restart in dropdown menu

@donavanbecker
Copy link
Contributor

are we going to have the same issue as .57? the code looks the same.

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

I split the code into 3 PR:

  • language fixes
  • confirmation
  • add username to menu (i thikk that here i made previously mistake)

so maybe first add lang fix and confirmation and we will test it?

@donavanbecker
Copy link
Contributor

okay all have been merged and upgraded with no issues so far. Should I merge this?

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Yes, add please. I think that problem was in username.

@donavanbecker donavanbecker enabled auto-merge (squash) November 9, 2023 05:51
@donavanbecker donavanbecker merged commit 216a74f into homebridge:beta-4.52.2 Nov 9, 2023
14 checks passed
@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Works well.

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Hmm... wait....

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

It showing modal to shutdown server - it should be to restart.

@donavanbecker
Copy link
Contributor

can you confirm all are working good?

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

No.

We need to replace

shutdownServer()

to

restartServer()

Will you do it or should I do PR?

@donavanbecker
Copy link
Contributor

done

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Thank You.

@donavanbecker
Copy link
Contributor

FYI there does seem to be an issue with install of new plugins. Can you please take a look?

@donavanbecker
Copy link
Contributor

Screenshot 2023-11-09 at 12 20 35 AM

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Hmm... It seems that nothing appears in this window - it's not about the fact that, for example, the text is invisible, but just as if nothing is happening there. In my PR there was rather nothing that would have any impact on it.

@donavanbecker
Copy link
Contributor

Yeah not on this one but I think one of them?

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Changing theme - not solve problem. When I inspect this element it is just empty. It looks like nothing is happening there.Because if it was a display problem, the window would disappear after a while. I also wonder why the plugin update works well but the installation of the new one does not?

My PR was about:

  • dropdown menu
  • default layout on the status page + small widget fixes
  • translations

I don't think anything will affect it. All the changes I proposed were only in the "ui" directory - I don't think he was responsible for installing plugins. But of course I will think, I will look, I will try to help.

@grzegorz914
Copy link
Contributor

grzegorz914 commented Nov 9, 2023

@donavanbecker
on my side with latest beta.63 install new plugin working correct

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

beta 63 - I have same problem as @donavanbecker mention

I have raspberry pi 4
Homebridge Image 1.0.41
Linux Bullseye
Node 20.9.0

@grzegorz914
Copy link
Contributor

grzegorz914 commented Nov 9, 2023

@mkz212 , @donavanbecker strange, on my side all working as expected

macOS Sonoma
macMini M1
node 21.1.0
hb 1.7.0

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

@donavanbecker And You? Maybe we will find a common point.

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

@grzegorz914

Do you see any more mistakes?

I think we've solved everything you've been writing about lately.:

  • restart with confirmation
  • scrollable homebridge widget
  • and other fixes in status page

@donavanbecker
Copy link
Contributor

Still having issue on latest beta with docker.

Last thing I am trying to implement before releasing this version is child bridge restart, if anyone want to take what I have added so far....

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

DOESN'T WORK ON:

  • Raspberry Pi 4B
  • Homebridge Image 1.0.41
  • Linux Bullseye
  • Node 20.9.0

WORK ON:

  • macOS Sonoma
  • macMini M1
  • node 21.1.0
  • hb 1.7.0

So I think that is not node or hb's fault.

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

@donavanbecker When i try to install plugin via terminal:
Zrzut ekranu 2023-11-9 o 08 00 12

@donavanbecker
Copy link
Contributor

That only happens to the bottom item

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

@donavanbecker

If it is not to appear, you have to add to css:

.dropup .dropdown-toggle:after {
display:none;
}

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

and this css is to bound to the right side (@grzegorz914 idea):

.card-body .dropdown-menu.show {
transform: translate(-200px, 30px) !important;
}
Zrzut ekranu 2023-11-9 o 08 31 09

But I don't think it's a good idea. Currently, the window adjusts dynamically. For example, on the mobile, in the last element when we click this window will appear above 3 dots:
Zrzut ekranu 2023-11-9 o 08 35 57

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Ok I made PR with a possible solution for it to be bounded to right and adjust automatically.

@mkz212 mkz212 deleted the patch-17 branch November 9, 2023 08:14
@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

@donavanbecker

If I go into the search for a new plugin, strange things happen:

  • empty fields appear at the bottom and in one plugin there is opened dropdown:
Zrzut ekranu 2023-11-9 o 13 56 45 - click on Install another version show empty modal: Zrzut ekranu 2023-11-9 o 13 57 35 - command to install plugin via terminal still doesn't work: Zrzut ekranu 2023-11-9 o 13 59 41

@donavanbecker
Copy link
Contributor

We need to fix that.

@grzegorz914
Copy link
Contributor

@mkz212
very strange, on my side all is correct
image
image

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

I checked it on different browsers (Safari and Firefox) And I have the same error on both. But it only occurs when installing a new plugin. In the view of the current plugins, everything works fine. If it works well for you, maybe it's a bug related to the device ? @donavanbecker @bwp91 Didn't you introduce anything related to the GLIBC and node error - something to detect system etc.?

In ui code there is one code responsible for the appearance of both: installed plug-ins and those to be installed. If there was an error here, it would be visible in both places, but this is not because the installed plug-ins are displayed correctly.

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

The problem occurs when the plug-in list is too long. E.g. If I only have two results or 5, it's ok - I can install new plugin and the layout is ok. But even if I have long list installed plugin - there is no problem - problem only when search for new plugin!

But what does it have to do with installing a plug from the terminal command? You can't install the plugin there.

And why for @grzegorz914 all works fine?

@donavanbecker
Copy link
Contributor

This resolved it for me! #1673

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

beta 66 still have this problem:
Zrzut ekranu 2023-11-9 o 16 23 58

@grzegorz914
Copy link
Contributor

grzegorz914 commented Nov 9, 2023

@mkz212 @donavanbecker if search for camera then result is 30 plugins and all is displayed correct but if search for samsung the result is same 30 plugins but only first 9 is displayed correct. Search other word like apple also display 30 plugins correct.

@donavanbecker
Copy link
Contributor

Okay I can recreated.

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

same here!

camera - ok
samsung - problem
apple - ok

@donavanbecker
Copy link
Contributor

donavanbecker commented Nov 9, 2023

must be a feature to push people to Apple? ;)

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

I just checked and in 4.52.1 it is ok.

@donavanbecker
Copy link
Contributor

pushing a potential fix that we found

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

beta 67 still have this problem

@donavanbecker
Copy link
Contributor

beta68 should fix it.

@donavanbecker
Copy link
Contributor

@mkz212 are you on the homebridge discord?

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

Finaly! Beta 68 fix it!

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 9, 2023

but still problem installing plugin from terminal:

Zrzut ekranu 2023-11-9 o 16 58 22

@grzegorz914
Copy link
Contributor

grzegorz914 commented Nov 9, 2023

@donavanbecker @mkz212 b.68 fix for me too, in other things after update plugin running as child bridge I get possibility to restart only the child bridge but nothing happens after press the button on modal window.

@donavanbecker
Copy link
Contributor

yeah that is a work in progress. Restart Homebridge should show as well right now.

@donavanbecker
Copy link
Contributor

but still problem installing plugin from terminal:

Zrzut ekranu 2023-11-9 o 16 58 22

if you are on hb-service you can do npm install

@donavanbecker
Copy link
Contributor

donavanbecker commented Nov 10, 2023

@mkz212 seeing this on docker:

image

and shouldn't be

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 10, 2023

@donavanbecker Can you be a little more accurate? When do you see it? And why shouldn't it be? In the sense, there should be no restart option at all? Shouldn't there be confirmation?

@donavanbecker
Copy link
Contributor

you can't restart the docker OS, you can however restart the container. we have reverted the restart back to just restarting homebridge for now.

@mkz212
Copy link
Contributor Author

mkz212 commented Nov 10, 2023

Aaaa... OK! Thank You. I have already corrected. I added PR. Now there is detection whether docker or not (as in the dropdown menu).

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.

3 participants