martyr-deepin / deepin-terminal-gtk

DDE terminal emulator application
GNU General Public License v3.0
263 stars 57 forks source link

Use alternative split char #205

Open ratcashdev opened 4 years ago

ratcashdev commented 4 years ago

to solve #204

BLumia commented 4 years ago

Good catch but it seems it will make the existed config file no longer works, maybe do a character escape like \@ and unsecape it when using them instead of simply calling .split("@"); can be a better approach?

ratcashdev commented 4 years ago

@BLumia not sure. Wouldn't an escaped \@ still count as a @ when splitting? I am afraid it will.

As for existing config files... you're right. I have added backward compatibility in the latest commit.

ratcashdev commented 4 years ago

I think having a | is far less likely and problematic than anything else. Plus I still don't see how escaping would help. Care to explain?

BLumia commented 4 years ago

I think having a | is far less likely and problematic than anything else.

We're glad you found the @ split char issue. But since we now know there can be issue, we should avoid any possible case which can cause the same issue like using | or :. So IMO "far less likely and problematic than anything else" is also not acceptable.

Plus I still don't see how escaping would help. Care to explain?

A escaped string can be username\@domain.com@host.domain@port. Then we need to implement a simple split() function to do split only when it found a @ instead of \@. But it can be a little complex if we need also treat the escaped character\\.

A much simpler way to implement (instead of using the escaped/unescaped approach) is just treat last two @ as valid split char as I already said before, so there will be no problem with the original config file format, and easy to implement.

In string.split()there is a max_tokens argument to use. Since we need the last two @ as valid ones instead of first one, we can do a string.reverse() first before we call split(). Then after the split() call, just reverse() the splited strings back will be okay.

// pseudo code, *not* real vala code.
// reverse the original string, split.
server_infos = server_info.reverse().split("@", 3);
// reverse the result list. strings in the list still need to be reverse(). 
server_infos.reverse();
// reverse the strings inside the list.
server_infos.foreach ((entry) => {
    entry.reverse();
});

Or other ways can also works like just simply split with @ and string join all entries without the last two string. Or maybe you have any better approach which also works :)

ratcashdev commented 4 years ago

Possibly. Or BASE64 encode username/host and then we can use split with the current char (@).

magynhard commented 3 years ago

@BLumia i like your suggestion, it is backward compatible and also solves the original problem as well, without users needing to know, that they have to use special characters in that case. I think it's the most intuitve solutation.

@ratcashdev BASE64 is an approach, but will not work with old configuration files - you may introduce some patterns so ensure if the file is encoded or not and to do that at the next time you save the file. So the solution from @BLumia seems be an quick win.