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

FF/kde-integration - Remove browser-kde.xul? #3

Open
msirringhaus opened this issue Jun 6, 2019 · 5 comments
Open

FF/kde-integration - Remove browser-kde.xul? #3

msirringhaus opened this issue Jun 6, 2019 · 5 comments
Assignees

Comments

@msirringhaus
Copy link
Contributor

Appyling openSUSE's FF67 KDE patches to FF68 beta worked, but the browser didn't start.

The problem was the file browser-kde.xul, which is created with the patch firefox-kde.patch.
That new xul-file is replacing browser.xul, which is part of the upstream repo.
I compared browser-kde.xul and browser.xul for FF67 on openSUSE and both are, except for one inversion of an ifdef (concerning a bookmark Done-button) identical.

I see no practical reason for browser-kde.xul to exist at all. Why create a new file that replaces the old one, when there is no real difference between them?

If that single difference concerning the bookmarks-button is important, we could consider patching the original file instead of copying its contents into the patch?

For FF68 browser.xul changed a lot. So I adjusted firefox-kde.patch to not create browser-kde.xul at all, but simply take the original one. I am omitting that single difference for now.
This seems to work fine ("Save as" opens the KDE file dialog for example).

Or is there some hidden magic I have overlooked that triggers more changes (maybe due to the filename being different?) when we replace that file with a new copy?

@wrosenauer
Copy link
Contributor

So you are right that the only difference between both files is one small change. This change changes the button order from Gnome-style to KDE-style. Maintenance-wise this is indeed a PITA but just patching the one file does not do the trick because we need both to make Firefox use one when running under KDE and the other one when running under any other desktop at runtime.
When the whole thing was introduced we didn't find a better solution to modify this XUL definition and so it has been kept that way. in JS code we can react dynamically but for XUL we failed to do it smarter.
The trick when creating the patch is typically copying the original file, change the one ifdef and refresh the patch.

@wrosenauer wrosenauer self-assigned this Jun 6, 2019
@msirringhaus
Copy link
Contributor Author

Thanks for the explanation!

I am going to reopen this ticket, because with FF69, browser.xul will be replaced by browser.xhtml.
There might then be a chance to get rid of it then.

@msirringhaus msirringhaus reopened this Jun 6, 2019
@wrosenauer
Copy link
Contributor

Currently browse-kde.xul is gone. Currently we do not patch browser.xhtml but not sure if we have to. I would like to do some KDE testing but so far at least nobody complained.

@msirringhaus
Copy link
Contributor Author

Is there any specification available, of what these patches actually do? Meaning, a list of the features being added and how they work (from a user-perspective)?

@wrosenauer
Copy link
Contributor

https://bugzilla.mozilla.org/show_bug.cgi?id=528510
This was the original feature set more or less.
For the XUL part it was basically just left

  • button reordering for the bookmark dialog
  • button reordering for the generic dialog.xml

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

No branches or pull requests

2 participants