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

Fixed DHTMLControls #1116

Merged
merged 3 commits into from Mar 1, 2016
Merged

Fixed DHTMLControls #1116

merged 3 commits into from Mar 1, 2016

Conversation

Bo98
Copy link
Contributor

@Bo98 Bo98 commented Jan 18, 2016

After the Awesomium functions and hooks now work/exist, this control can now be fixed. It was pretty much broken at everything except entering URLs into it.

Side notes about the reasoning behind some of the changes:

  • self.Refreshing was replaced with a URL check as such check was required to prevent a double history insertion on setup. If the controls were hooked before the initial page began loading (likely scenario as :OpenURL(...) doesn't start loading immediately) then the history would be appended on setup and again on load. Instead of removing the initial insertion, a history check was added in case someone decided to add the controls later. This also covers refreshes so that special boolean isn't required now.
  • self.NavStack was useless and is now removed.

@bmwalters
Copy link
Contributor

A+ commit 👍+10

edit: Actually, after playing around with this in normal use scenarios, the history buttons seem quite glitchy to me. Have you tried this out?

@Bo98
Copy link
Contributor Author

Bo98 commented Feb 3, 2016

Works fine for me. In what case is it glitchy for you?

@bmwalters
Copy link
Contributor

Alright, so there aren't the bugs I'd thought I'd found.
Some flaws though:

  • Refreshing shouldn't add a new history entry
  • When you refresh, instead of disabling the button, replace it with the "cancel loading" button as most browsers do. Then this button can be remoevd from the right side.

@Bo98
Copy link
Contributor Author

Bo98 commented Feb 3, 2016

  • Refreshing shouldn't add a new history entry

Doesn't happen for me.

  • When you refresh, instead of disabling the button, replace it with the "cancel loading" button as most browsers do. Then this button can be remoevd from the right side.

I didn't touch this but I agree that it's retarded so I'll add a commit for that.

@Bo98
Copy link
Contributor Author

Bo98 commented Feb 3, 2016

There is a history corruption issue with fragment identifiers (the # part of some URLs that let you jump to sections - like on Wikipedia TOCs) but it is impossible to fix currently without constantly (eg. Think hook/timer) checking the URL property.

@willox Can we get Awesomium::WebViewListener::View::OnChangeAddressBar? Maybe even Awesomium::WebView::CanGoBack and CanGoForward too so we don't need to have our own copy of the history. Not sure whether CEF is anywhere near ready or not.

@Bo98
Copy link
Contributor Author

Bo98 commented Feb 3, 2016

  • When you refresh, instead of disabling the button, replace it with the "cancel loading" button as most browsers do. Then this button can be remoevd from the right side.

Added now.

@bmwalters
Copy link
Contributor

This will need to be updated for http://wiki.garrysmod.com/page/Panel/StopLoading for the next update.

@Bo98
Copy link
Contributor Author

Bo98 commented Feb 8, 2016

Done.

@bmwalters bmwalters mentioned this pull request Feb 17, 2016
robotboy655 added a commit that referenced this pull request Mar 1, 2016
@robotboy655 robotboy655 merged commit 0ea6ce3 into Facepunch:master Mar 1, 2016
@Bo98 Bo98 deleted the dhtmlcontrols-fix branch March 1, 2016 15:47
@Bo98
Copy link
Contributor Author

Bo98 commented Mar 28, 2016

@zerfgog The merging of the stop and refresh buttons unfortunately won't happen (and thus is reverted) since apparently some addons relied on the internal view hierarchy of DHTMLControls for some reason and broke when the buttons were merged.

@bmwalters
Copy link
Contributor

@Bo98 😐 that's quite unfortunate

@Kefta
Copy link
Contributor

Kefta commented Mar 28, 2016

Put a warning out next update, deprecate the behaviour, merge your PR

@Bo98
Copy link
Contributor Author

Bo98 commented Mar 28, 2016

This control is eventually going to be deprecated and replaced in its entirety when CEF is implemented so we can wait until then and make sure the changes are there for the new control.

At least the critical bug fixes are merged - it was only the button merging part that was reverted.

robotboy655 added a commit that referenced this pull request Mar 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants