[Phpmyadmin-devel] Issues with refactoring of tbl_structure

[Marc Delisle]

Le 2012-08-17 22:34, Thilina Buddika Abeyrathna a écrit :
> On Sat, Aug 18, 2012 at 1:08 AM, Marc Delisle <marc at infomarc.info> wrote:
> 
>> Thilina Buddika Abeyrathna a écrit :
>>> On Fri, Aug 17, 2012 at 6:35 PM, Rouslan Placella <rouslan at placella.com
>>> wrote:
>>>
>>>> On 17/08/2012 13:17, Thilina Buddika Abeyrathna wrote:
>>>>
>>>>>
>>>>> On Fri, Aug 17, 2012 at 4:31 PM, Rouslan Placella <
>> rouslan at placella.com
>>>>> <mailto:rouslan at placella.com>> wrote:
>>>>>
>>>>>     On 17/08/2012 12:00, Thilina Buddika Abeyrathna wrote:
>>>>>      >
>>>>>      >
>>>>>      > On Fri, Aug 17, 2012 at 1:42 PM, Rouslan Placella
>>>>>     <rouslan at placella.com <mailto:rouslan at placella.com>
>>>>>      > <mailto:rouslan at placella.com <mailto:rouslan at placella.com>>**>
>>>>> wrote:
>>>>>      >
>>>>>      >     Hi all,
>>>>>      >
>>>>>      >     I've found some obviously broken HTML tags on
>>>>>     tbl_structure.php and
>>>>>      >     I assume that this is due to the recent refactoring of this
>>>>>     page. I
>>>>>      >     couldn't find where the issue is in the code, but it
>> manifests
>>>>>      >     itself in the "more" dropdown as links there are now not
>>>>>     clickable.
>>>>>      >     Screenshot attached.
>>>>>      >
>>>>>      >     Another thing that I noticed in the new *_structure code are
>>>>>     the new
>>>>>      >     function names. Not sure if it's just me, but after a 2
>>>>>     minute look
>>>>>      >     I found all of these functions where IMO the names can be
>>>>>     improved:
>>>>>      >
>>>>>      >     PMA___**getHtmlForPrintViewAndDataDict**__ionaryLinks - too
>>>>> long?
>>>>>      >     PMA_SortableTableHeader - starts with a capital unlike
>> others
>>>>>      >     PMA_getAliasAndTruename - incorrect camel case
>>>>>      >     PMA_getStuffForEnginetable - meaningless + incorrect camel
>> case
>>>>>      >     PMA___**getHtmlForCheckAlltableColumn - incorrect camel case
>>>>>      >     PMA_getHtmlForSomeLinks - meaningless
>>>>>      >     PMA_getHtmlForRowStatstableRow - incorrect camel case
>>>>>      >     getHtmlForRowStatsTable - no PMA_ prefix
>>>>>      >     PMA___**getHtmlForDistincValueAction - spelling: "Distinct"
>>>>>      >     PMA___**getHtmlForActionsIntableStruct**__ure - incorrect
>>>>> camel case
>>>>>      >
>>>>>      >     Bye,
>>>>>      >     Rouslan
>>>>>      >
>>>>>      >
>>>>>      > Hi  Rouslan,
>>>>>      > Thank you for spent time to look at my code. And I'll improve
>> those
>>>>>      > function names,
>>>>>      > And I'll look at the mentioned bug also.
>>>>>
>>>>>     The bug is an unclosed div tag at line 1831 in structure.lib.php
>>>>>
>>>>> Thanks  Rouslan.
>>>>>
>>>> Hi Thilina,
>>>>
>>>> There seems to be a whole bunch of other bugs on the table structure
>> page
>>>> that have to do with the dropdown menu. It would be great if you could
>> look
>>>> at this asap as these have caused breakages in my branch.
>>>>
>>>> To reproduce, create a table with the following definition:
>>>>
>>>> CREATE TABLE IF NOT EXISTS `gfdfgd` (
>>>>   `a` int(11) NOT NULL,
>>>>   `s` linestring NOT NULL,
>>>>   `f` text NOT NULL,
>>>>   PRIMARY KEY (`a`)
>>>> ) ENGINE=MyISAM DEFAULT CHARSET=latin1 ROW_FORMAT=COMPACT;
>>>>
>>>> Then, please look at the attached screenshots.
>>>> In screenshot-1, the fulltext link is missing from the text column.
>>>> In screenshot-2, both the fulltext and spatial links are missing.
>>>> In screenshot-3, you can see an unwanted link in the top left and a
>>>> fulltext link for a spatial column.
>>>> In screenshot-4, you can see an spatial link for a text column.
>>>>
>>>> I didn't do a bisect...
>>>>
>>>>
>>> Thank you very much  Rouslan. I'll look at those issues.
>>
>> I see that Thilina has fixed most of these problems, except that the
>> More drop-down is missing some of the links and has incorrect other
>> links (for example, a linestring column should not have a link to add a
>> FULLTEXT index).
>>
>> Fixed in cd2177ec4b0fa58be20ca563cc7bcbd645126b93 commit.

Looks fine. Pushed to upstream, thanks.


-- 
Marc Delisle
http://infomarc.info