Skip to content

Add manual links to controllers#3108

Merged
daschuer merged 8 commits into
mixxxdj:2.3from
Holzhaus:controllers-manual-link
Sep 23, 2020
Merged

Add manual links to controllers#3108
daschuer merged 8 commits into
mixxxdj:2.3from
Holzhaus:controllers-manual-link

Conversation

@Holzhaus
Copy link
Copy Markdown
Member

No description provided.

@Holzhaus
Copy link
Copy Markdown
Member Author

Requires mixxxdj/manual#122 to be merged first.

Copy link
Copy Markdown
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I have left some comments.

Unfortunately this is too big to review.
Can we revert the mass reformation?

@Holzhaus Holzhaus force-pushed the controllers-manual-link branch from 0a76464 to 3d181ac Compare September 20, 2020 19:18
@Holzhaus
Copy link
Copy Markdown
Member Author

Unfortunately this is too big to review.

I removed the commit. Btw, you can hide files by type in the "Files Changed" tab: https://github.com/mixxxdj/mixxx/pull/3108/files?file-filters%5B%5D=.cpp&file-filters%5B%5D=.h

@Holzhaus Holzhaus requested a review from daschuer September 20, 2020 22:46
@Holzhaus
Copy link
Copy Markdown
Member Author

Moving the discussion from a removed (now orphaned) commit here for better visibility:

@daschuer

Can we auto generate a manual link?
I can imagine that this will help to create a manual page.

@Holzhaus

Can you explaint what I have in mind? The manual link is already partially auto-generated. You only need to specify the path, not the full URL.

@daschuer

We may use the mapping file name or something..

@Holzhaus

I don't think that's a good idea. Our controller mapping filenames have a very weird naming scheme (mixture of upper/lowercase chars, mixture of underscore and dashes, includes spaces). Some controller mappings include the manufacturers name, some don't. Sometimes the mapping creator appended his/her initials to the mapping filename and we failed to catch this during review.

Unfortunately, we can't change mapping names without breaking backwards compatibility (controller mapping would be unassigned).

For the controller docs in the manual, I tried to use the name of mappings exactly as used by the manufacturer, including the company name and just amde them lowercase converted special chars and spaces into underscores.

@daschuer

Here we are just talking about a default for new mappings.
I think if the mapping file has a nice name, it is reasonable to generate the manual from it by default. This avoids namespace conflicts. If not we can specify any other name.

Just an idea. if you still don't like it, it is OK for me.

@Holzhaus
Copy link
Copy Markdown
Member Author

Holzhaus commented Sep 21, 2020

Here we are just talking about a default for new mappings.
I think if the mapping file has a nice name, it is reasonable to generate the manual from it by default. This avoids namespace conflicts. If not we can specify any other name.

What do you mean by "namespace conflicts"? Can you give an example?

IMHO we could start estabilishing a new naming scheme for controller mappings to be able to do that. I'd appreciate that, but maybe we should also postpone that until the new controller system is in place and keep the naming scheme for legacy mappings for consistency.

Also, I think we'd need another mechanism to distinguish user controller mappings from built-in ones. Right now, it's easy, because you just don't specify a manual page in custom mappings. If we auto-generate the link, we'd need to make an HTTP request to check if the page exists.

I can imagine that this will help to create a manual page.

I have doubts about this. For the Wiki page that would make sense, because you could just click the URL and be presented with an editing page. This is not the case for the Manual.

Done using this python script:

    import re
    import requests
    import sys

    def replace_forum_url(matchobj):
        oldurl = matchobj.group(0).decode().replace("&", "&")
        oldurl = "https://mixxx.discourse.group" + oldurl.partition("forums")[2]
        print(oldurl)
        req = requests.head(oldurl, allow_redirects=True)
        return req.url.replace("&", "&").encode()

    for file in sys.argv[1:]:
        with open(file, mode="rb") as f:
            content = f.read()

        content = re.sub(rb'https?://mixxx.org/forums/[\w\?&;=\/\.]+', replace_forum_url, content)

        with open(file, mode="wb") as f:
            f.write(content)
Comment thread res/controllers/Vestax VCI-300.midi.xml Outdated
@daschuer
Copy link
Copy Markdown
Member

Thank you. LGTM

the pre-commit hook reports some white space issues.
Lets ignore them for now to not clutter this PR with unrelated changes.

@daschuer daschuer merged commit 64354bd into mixxxdj:2.3 Sep 23, 2020
@Holzhaus
Copy link
Copy Markdown
Member Author

the pre-commit hook reports some white space issues.
Lets ignore them for now to not clutter this PR with unrelated changes.

Yes, I had to skip a bunch of hooks because the formatting of our XML files is completely broken. This is why I opened #3107.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants