-
Notifications
You must be signed in to change notification settings - Fork 12
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
Targets with invalid character encoding (was: Maximum length of URLs?) #76
Comments
Thanks for the feedback! |
I can reproduce an issue with creating a new Shorty using the URL you provided as target. Targets can have a length up to 4096 bytes according to the implementation. The resulting length is actually much lower due to the usage of unicode to store idn domains and the like. But still this limit should not even closely be hit here. I currently have no idea what the problem is. I will have to investigate. |
Sorry, this is an issue with the IDN handling I implemented. That is quite buggy, my fault. |
Thanks for your quick reply! I didn't really suspect the length itself causing the issue. The title was just remaining from the first glance, because unusual long URLs seemed to be problematic. Of course it would be kind of funny if a URL-shortener would be overwhelmed by being fed with long URLs 😀 I'm sorry I can't give any better hints because js is not by business. But maybe some of the chars within the URLs are the cause (#, %, ?, ...) Here's another one: As a conclusion: Would you suggest to switch back to an earlier version of Shorty? Thanks, |
Hi! On Sunday 28 December 2014 07:52:33 fredl99 wrote:
Sure, you can downgrade, you can pick it from github. arkascha |
OK, this turned out to be two issues combined, which made debugging somewhat annoying:
|
@fredl99 Could I ask you to help testing the upcoming versions? |
Sure I'm willing to help. I'm going to install it and will report. (Didn't know when you plan to release 0.5, so of course I can wait 👍 ) |
Ok, did it and 0.5.0 seems to work fine.
Ok, I recognized the "slider" is actually a "click/double-click" element. Sorry... I have not evaluated shorty_tracking 0.3.0 by now. Only one question: shorty and shorty_tracking are in a common directory now, which also contains another subfolder "l10n". Is that on purpose with a special meaning? |
One more thing: Feature request: Wouldn't it be nice to have the table sorted by clicking on a header field? (Instead of shrinking/expanding the according column) Please feel free to cut and move this testing thread once again. |
OK, I understand:
About clicking a column title to sort it: |
@fredl99 Another thing: about the "1970-01-01 01:00:00" for the "last access" date: |
I must admit I'm not very familiar with Shorty. In general, my owncloud-server is not as heavily used as it should be. But I'm working on it as you can see. Once I started to play with it, the problems came along. (Not really, only joking!) So, thank you for pointing me to the triangle. Meanwhile, by switching a bit back and forth between 0.4.4 and 0.5.0 I discovered the sort option in 0.4.4 and I think I like it more. At least I discovered it by myself... Filtering is cool indeed, but I like sorting, too. One can see the popularity of Shortys at one glance. Or which ones are old and outdated. That's useful imo. As for the database, I'm using MySQL. The access date isn't such an issue, as it's corrected after the first access. One more thing: When I edit a Shorty, it's entry is showing "undefined" in title and target fields, and "never" in expiration, creation and last date fields. But it still works and is correct again after reloading the page. 0.4.4 doesn't do that. Didn't know about the common repo / folder hierarchy. I always had a standard install of OC with Shorty enabled. At least in the latest versions there where only those two folders within the "apps" directory. ATM I'm just switching symlinks between "dist" and "test" folders outside of "apps". |
I fix the visualization of modified Shorty directly in the lists, that was indeed broken. Thanks!
Your feedback is really helpful, thank you so much! |
I have to thank for the quick solutions. BTW: I have cloned the github branch "stable7", so I can quickly fetch new commits for testing. Just let me know. |
I merged a few fixes. Amongst it a fix for the "umlauts issue" in the title aspect. Turned out the original strategy was quite ok, so I reverted some details again. Actually the title tag inside your example page at http://www.willhaben.at/.... is invalid! It contains an invalid utf sequence which made my title extraction routine crash. I made my routine more robust, so that it ignores such invalid titles now. Seems to work. Impressive quality that "willhaben.at" service offers. Been a while since I last saw a professional service unable to handle character encoding :-) Could you give this a try and test if umlauts work for you now? |
Looks much better now!
That's all I saw by now :) |
Oh, I forgot: |
Great, what a wealth of information! Priceless! Are you for hire? :-)
Thanks again! |
Thank you for the explanation of the title problem. I see, it's more complicated than somebody whose coding skills end at HTML3/4 and CSS2 would expect 😄 Regarding the bookmarklet you shouldn't waste any time. As I stated it still works with Iceweasel/Firefox and I also checked with Qupzilla now. The 80 chars boundary is funny: When a correct title is already filled in, chars can be deleted but not appended again. But it's completely there at the beginning. When I try copy&pasting, it is truncated to 80 chars. |
N.B.:
It's only in the list. 0.4.4 shows correct dates, even when they where altered by 0.5.0 before. |
@fredl99 Which repository did you clone to update your test environment:
Sorry, cannot see that in github. I ask because I just wonder where to publish my changes for you to test. |
Fixed:
Unbelievable how many regressions were introduced by the DOM changes in 0.5. |
I cloned from https://github.com/owncloud/shorty. I thought this is what you were referring to earlier If you have some spare time: Could you check if there could be some occasional error in parsing URLs of favicons? I have collected two examples which obviously fail due to a missing slash: Going to pull your fixes... |
Still have to look at shorty_tracking. |
About the table filtering: not sure of the entry should vanish in cases you describe. This is an ages old discussion. The problem: The entry you edit vanishes and the user has no feedback at all and cannot revert the change. That constantly leads to confusion and frustration. |
I just merged a reimplementation of the "collapsible column feature". |
About the favicons and the example sites you gave: OK, pushed a small fix dealing with this, maybe things are better now for you. |
Changing the expiration date of an existing Shorty results in a correct display as date in the reloaded list afterwards. I never get a timestamp here, actually that aspect is a date, no time associated. No idea how that can come out as a timestamp. Ah! I just realize you probably confuse the columns! OK, seems to be fixed too now :-) |
I think this thread is becoming a bit confusing now. Can you divide it into separate topics again, please?
They do, but the URL in Shorty's table is missing the last slash:
There's a visual difference between the "empty icon" provided by Shorty and a "missing graphics" icon provided by the browser: Right click on the icon and "open picture in new tab" or "copy picture URL" reveals the truth 😄 I think I know the reason: Both of these sites use relative links: So if the base URL would end in a slash in the two examples, the favicon-URL would work, too. |
Filtering is not my main interest, so I'm fine as it is anyway. About the collapsible columns, I will have a look at it, but just as a thought:
Of course I understood the intention behind this feature. The purpose is to give more room for a particular column. So why not make it a single-click action: i.e. clicking the "target" header collapses all other columns to their minimum width at once and expands the "target" column to the remaining space? A second click reverts all columns to their defaults. Imho that would be nicer than repeatedly collapsing columns until the desired one gets enough room (and still won't be showing the entire content sometimes). Is this technically possible? |
Yes, I think it's more intuitive this way. Thanks!
A darker triangle is the usual way to emphasize the sorting order, if any. It wouldn't have to be bigger too. Regarding the single-click expansion mentioned before: The "double-arrow" mouse-pointer would also describe this action better than it does now. |
Re: Last access date
You're right, my explanation was misleading. Regardless of the change, the time of the last access was shown in seconds of the Unix epoch afterwards. As I've tried it with the expiration date, I reported it wrong.
It is. ✅ |
Since this is fixed I won't separate it into a separate issue here. |
|
I will experiment with different systems and browsers. I hesitate to generate a custom cursor for this... |
Let's see if there is a way to get both ideas into a single solution :-) |
favicons:
Would have been a cheap opportunity to add to the "fixed issues" 😆
In no way I want to sound ignorant. But most browsers/OSs offer high contrast settings these users will
Yes, same here. And for me it suggests "expand". That's why I meant it would fit to "expand this column to the max" by collapsing all others. Naturally another pointer would be nice for reverting all back to the last state (replace "default widths" as I wrote before, because I didn't consider that these settings are stored somewhere.) |
Ah, I see what you mean with the cursor: currently the hovering cursor is the same for both actions: collapse and expand. Indeed misleading. |
About separating issues: Though certainly getting feedback, questions and hints on issues at all is most important :-) |
Hm, doesn't expanding a column to max and collapsing all other columns only make sense for the target and title column, if at all? |
Sure. That's were I use this feature anyway. :) |
A bit on the initial topic. I just saw this:
Do you need any additional input for this? |
I'd say that is the error message thrown by php when trying to process the title with the invalid encoding used by willhaben.at I mentioned 4 days ago in this thread. I implemented a workaround for such issue and for me it works. But that does not mean that the sequence is not illegal any more. It is just ignored now :-) |
I'm sorry, it's from "2015-01-02T13:18:26+00:00" But I'm ready to ignore it too ;) This one is around for a long time and it's still here: |
Yes, i know that message is still thrown. As said: it is ignored now (the issue) which was not the case before. I cannot change the way willhaben.at encodes their title tag. So this error will always be thrown, also in future, when such an illegal string is processed. I separated the issue with the "undefined function p()" as #90. |
I renamed this issue reflecting the real problem behind it. |
Re: Shorty 0.4.4, ownCloud 7.0.4 (stable)
Hi,
Trying to manually shorten a URL like this one:
http://www.willhaben.at/iad/kaufen-und-verkaufen/kfz-zubehoer-motorradteile/audi-a6-c4-seitenspiegel-beide-107995093?adId=107995093
Shorty complains: "Sorry, that target is invalid!"
When using the Bookmarklet it results in an "Internal Server Error (500)" :(
When I manually cut away the portion from the righmost slash up to end of the URL then Shorty will accept it. But that's no useful target then.
Second: The selected states don't seem to work properly anymore. Any of "closed", "private" or "shared" results in a "forbidden" message when calling the short URL, regardless if or which user is logged in. No login-page appears either. The only working status at the moment is "public".
The text was updated successfully, but these errors were encountered: